-
-
Notifications
You must be signed in to change notification settings - Fork 5.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Deprecate symbol (to Symbol) #16154
Deprecate symbol (to Symbol) #16154
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
From comments by stevengj in #15995.
- Loading branch information
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -455,14 +455,14 @@ end | |
for T in indexes.parameters | ||
T <: CartesianIndex ? (M += length(T)) : (M += 1) | ||
end | ||
index_length_expr = index <: Colon ? Symbol(string("Istride_", N+1)) : :(length(index)) | ||
index_length_expr = index <: Colon ? Symbol("Istride_", N+1) : :(length(index)) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah damn, this'll probably fail. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. also best not to at-notify users in commit messages, otherwise every time this gets rebased they will get a notification There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good to know, will change the message when I rebase. Er, why is this "outdated" already?! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I take it back, the above should work. Do you want me to amend the commit message (to remove the mistyped stevengj's at handle)? |
||
quote | ||
Cartesian.@nexprs $N d->(I_d = indexes[d]) | ||
dimlengths = Cartesian.@ncall $N index_lengths_dim V.parent length(V.indexes)-N+1 I | ||
Istride_1 = 1 # strides of the indexes to merge | ||
Cartesian.@nexprs $N d->(Istride_{d+1} = Istride_d*dimlengths[d]) | ||
idx_len = $(index_length_expr) | ||
if idx_len < 0.1*$(Symbol(string("Istride_", N+1))) # this has not been carefully tuned | ||
if idx_len < 0.1*$(Symbol("Istride_", N+1)) # this has not been carefully tuned | ||
return merge_indexes_div(V, indexes, index, dimlengths) | ||
end | ||
Cartesian.@nexprs $N d->(counter_d = 1) # counter_0 is the linear index | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should really not be combined with renaming
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's in the second, cleanup, commit.
Edit: That is, the right hand side of this line diff is in the second commit (removing
:end
). The symbol -> Symbol is in the first commit.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there are multi character dot operators that this loop may want to be extended to, I think this change is unnecessary and could make this bug prone later