-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
Prevent TOML invalidation by pirates of T[elements...]
#39252
Conversation
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.
Thanks Tim! :)
It may be worth considering adding comments for these types of transformations, such that those who come later don't revert the rewrites, not understanding the upside of the more complex forms.
StaticArrays (perhaps among others) semi-pirates this method, although they've worked hard to make it pretty innocuous. Still, there are times when it invalidates the TOML-reading, so best to protect this.
904fa74
to
48061b4
Compare
Done. |
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.
oops, I just assumed I improved the risk of invalidations when I modified this part last time.. Thanks for fixing this !
@@ -657,13 +657,20 @@ end | |||
######### | |||
|
|||
function push!!(v::Vector, el) | |||
# Since these types are typically non-inferrable, they are a big invalidation risk, |
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.
Fantastic comment! :)
# Since these types are typically non-inferrable, they are a big invalidation risk, | ||
# and since it's used by the package-loading infrastructure the cost of invalidation | ||
# is high. Therefore, this is written to reduce the "exposed surface area": e.g., rather | ||
# than writing `T[el]` we write it as `push!(Vector{T}(undef, 1), el)` so that there |
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.
tiny nit, but push!(Vector{T}(undef, 1), el)
should be push!(Vector{T}(undef, 0), el)
or the version in the code, right?
StaticArrays (perhaps among others) semi-pirates this method, although they've worked hard to make it pretty innocuous. Still, there are times when it invalidates the TOML-reading, so best to protect this. (cherry picked from commit 985dfa5)
StaticArrays (perhaps among others) semi-pirates this method, although they've worked hard to make it pretty innocuous. Still, there are times when it invalidates the TOML-reading, so best to protect this. (cherry picked from commit 985dfa5)
…9252) StaticArrays (perhaps among others) semi-pirates this method, although they've worked hard to make it pretty innocuous. Still, there are times when it invalidates the TOML-reading, so best to protect this.
…9252) StaticArrays (perhaps among others) semi-pirates this method, although they've worked hard to make it pretty innocuous. Still, there are times when it invalidates the TOML-reading, so best to protect this.
StaticArrays (perhaps among others) semi-pirates this method, although they've worked hard to make it pretty innocuous. Still, there are times when it invalidates the TOML-reading, so best to protect this. (cherry picked from commit 985dfa5)
StaticArrays (perhaps among others) semi-pirates this method,
although they've worked hard to make it pretty innocuous.
Still, there are times when it invalidates the TOML-reading,
so best to protect this.