minor code quality improvements #446
Conversation
aviatesk
force-pushed
the
minorimprovements
branch
from
67f4135 to
dff1860
Compare
Codecov Report
@@ Coverage Diff @@
## master #446 +/- ##
==========================================
- Coverage 89.75% 89.69% -0.06%
==========================================
Files 12 12
Lines 2322 2330 +8
==========================================
+ Hits 2084 2090 +6
- Misses 238 240 +2
Continue to review full report at Codecov.
|
|
CI failures on nightlies are not related to this PR; #449 will fix them anyway. |
| _getfile(ln::LineTypes) = ln.file::Symbol | ||
| _getfile(ln::Expr) = ln.args[2]::Symbol # assuming ln.head === :line | ||
| return CodeTracking.maybe_fixup_stdlib_path(String(_getfile(ln))) | ||
| end |
There was a problem hiding this comment.
Sorry for pushing such minor optimizations, these changes just circumvents inference limitations around JuliaLang/julia#37342.
There was a problem hiding this comment.
Getting rid of invalidation risk is very worthwhile, no apologies needed!
My only concern is again the loss of validation that this is really an Expr of the expected type.
src/utils.jl
Outdated
| @@ -295,7 +295,8 @@ function lineoffset(framecode::FrameCode) | |||
| return offset | |||
| end | |||
|
|
|||
| getline(ln) = Int(isexpr(ln, :line) ? ln.args[1] : ln.line)::Int | |||
| getline(ln::LineTypes) = ln.line::Int | |||
| getline(ln::Expr) = ln.args[1]::Int # assuming ln.head === :line | |||
There was a problem hiding this comment.
The original tries to convert to an Int and then asserts it succeeds; this change requires that they start out as Int. It also doesn't check that ln is a :line Expr/ Are you sure these changes are safe?
There was a problem hiding this comment.
I found the :line field of LineInfoNode and LineInfoNode are Int, but I didn't track how :line expression is constructed...
I think making explicit conversion here is way more robust and doesn't hurt anything, so let me do that again.
As for the validation for :line expression, I excluded that because I believe we're actually only calling this function on :line expressions, and non-:line Exprs will throw anyway (since it never has .line field). We can do something like getline(ln::Expr) = isexpr(ln, :line) ? ln.args[1] : throw("expected :line expression"), but I feel it's too much ?
| _getfile(ln::LineTypes) = ln.file::Symbol | ||
| _getfile(ln::Expr) = ln.args[2]::Symbol # assuming ln.head === :line | ||
| return CodeTracking.maybe_fixup_stdlib_path(String(_getfile(ln))) | ||
| end |
There was a problem hiding this comment.
Getting rid of invalidation risk is very worthwhile, no apologies needed!
My only concern is again the loss of validation that this is really an Expr of the expected type.
aviatesk
force-pushed
the
minorimprovements
branch
from
dff1860 to
bdc133c
Compare
|
Thanks! |
eliminates some inferrability issues, and also removes some real errors around
GlobalRef