Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
reduce some codegen costs #36600
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
reduce some codegen costs #36600
Changes from all commits
2ca0cc9
b77f400
f7adc9e
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Do you want to catch tuple and vector and matrix literals where the head is
vect
tuple
etc,and the args are all themselves
issimple
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.
not really, no
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.
Ideally I think we should avoid any nontrivial code in the final fallback code path of message formatting — even formatting the exception — as that itself could cause another exception. The point is that the application should never crash due to broken message formatting.
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.
No, we can't just keep going deep down the error chain. Eventually you have to admit you've broken everything.
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.
The point is to fall back to emitting a message which can't depend on user-defined code (formatting
"$ex"
can still run user code).We can clearly do this. I think you're asserting that you don't want to?
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.
Your "correction" still depends on user-defined code being valid, so, yes, clearly we can't do that. The subsequent lines of code need to run yet more user-defined code (and more complex code) so there's little point in adding try/catch here while forgetting to handle most of the actual sources of errors.
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.
Seems we're talking past each other here in some way, because I don't understand this statement.
Let me attempt to spell out my reasoning very explicitly.
The following code formats potentially user defined types in both branches, and these arise from the logging macro
The following invocation of user-defined code is related to the installed logger (which may internally protect itself with a try-catch of its own, should it so choose)
So to me it seems there's a clear separation of the user code which
and the former is what I'm arguing should be wrapped in various stupidly-nested try-catch statements.
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.
"protect" is just a relatively useless concept here, so I'm intentionally not attempting to pretend to do it excessively
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.
Well I don't understand that comment; saying it's a "useless concept" just leaves me guessing here which is a bit disappointing. I want to know the why here, not what.
But anyway, the actual technicality here is not a hill I'm willing to die on.
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.
Why remove
maxlog=nothing
from here? It seems to make the code below rather more complicated.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.
Saves the compiler/frontend from needing to create a keyword sorter and corresponding structdiff methods
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.
I don't understand why this is worthwhile... unless something about this API forces the amount of compiler work to scale with the number of logging statements in the program?
Is the point that
kwargs
can have many different types (one per logging statement) and that leads to trouble?If so that connects to another idea I've been thinking about: That perhaps we should be using a data structure more like
Dict{Symbol,Any}
, orImmutableDict{Symbol,Any}
for passing the key-value pairs of log records rather than (ab)using keyword arguments?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.
With this change, kwargs can already be (already is) an AbstractDict{Symbol,<:Any}, without making any further changes to the code