-
Notifications
You must be signed in to change notification settings - Fork 39
Make error messages lazy strings #420
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
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #420 +/- ##
==========================================
- Coverage 99.91% 99.82% -0.09%
==========================================
Files 8 8
Lines 1147 1154 +7
==========================================
+ Hits 1146 1152 +6
- Misses 1 2 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| @inline function setindex!(F::AbstractFill{T, N}, v, kj::Vararg{Integer, N}) where {T, N} | ||
| @boundscheck checkbounds(F, kj...) | ||
| v == getindex_value(F) || throw(ArgumentError("Cannot setindex! to $v for an AbstractFill with value $(getindex_value(F)).")) | ||
| v == getindex_value(F) || throw(ArgumentError(LazyString("Cannot setindex! to ", v, " for an AbstractFill with value ", getindex_value(F), "."))) |
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 not use the @lazy_str macro?
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 wanted to avoid the additional overhead. I also had the impression that in Base usually LazyString is used for error messages, not @lazy_str - I assumed also to avoid overhead.
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.
Usually that's because of bootstrapping issues, as some parts of Base are loaded before the macro is defined. Packages don't have the same issue. I imagine the overhead would be minimal.
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.
Ah true, that could be another explanation.
The overhead might be small but to me it seemed strange to provide the information/variables in a form that required string processing and parsing: https://github.com/JuliaLang/julia/blob/master/base%2Fstrings%2Flazy.jl#L67-L77
Based on #419 since
LazyStringandlazy"..."are only available in Julia >= 1.8.The PR changes all error messages to lazy strings to delay string interpolation and
printing of the involved quantities until the error message is displayed (and hence skip it completely if the error path is not hit). UsingLazyStrings in error messages generally also helps with type inference (see e.g. JuliaLang/julia#58672 and JuliaLang/julia#54737).