Remove fl_julia_current_{file, line} #53797
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
These flisp accessor functions make use of the `jl_filename` and `jl_lineno` globals. I was looking at removing these globals for unrelated reasons, when I saw that one of the primary uses was in these flisp accessor, which appear entirely unnecessary. They are only used to provide a default for an error message, but the place that puts the error message into a list to return to julia does actually already have access to this information, so there's no need for these to look at the globals. While we're at it, also add a test for this code path, which was otherwise unexercised in our test suite.
Keno
added a commit
that referenced
this pull request
Mar 21, 2024
I don't like `jl_filename`/`jl_lineno`. They are weird internal state, and they are also not thread safe, so if different threads are evaling different things at the same time, line numbers can get confused. This PR changes the core function `jl_toplevel_eval_flex` to keep track of its current file/line context on the stack, so at least there is no confusion within one call to this function. With this PR and #53797, the global `jl_filename`/`jl_lineno` are used for three purposes: 1. To initialize the filename/lineno used by lowering from `Core.eval`. 2. To give binding deprecation warnings. 3. For `jl_critical_error`. 4. By humans in the debugger. I think 3 and 4 are fine, they are exceptional cases. Case 2, I think could be changed to plumb through locations explicitly, but it's a bit annoying, so I didn't tackle it here. Case 1, I think can probably just be changed to consistently initialize, and if you want a proper line number, you need to put it in there explicitly. However, I didn't change that in this PR, because I think it could be slightly breaking, so should be pkgeval'd.
Keno
added a commit
that referenced
this pull request
Mar 22, 2024
I don't like `jl_filename`/`jl_lineno`. They are weird internal state, and they are also not thread safe, so if different threads are evaling different things at the same time, line numbers can get confused. This PR changes the core function `jl_toplevel_eval_flex` to keep track of its current file/line context on the stack, so at least there is no confusion within one call to this function. With this PR and #53797, the global `jl_filename`/`jl_lineno` are used for three purposes: 1. To initialize the filename/lineno used by lowering from `Core.eval`. 2. To give binding deprecation warnings. 3. For `jl_critical_error`. 4. By humans in the debugger. I think 3 and 4 are fine, they are exceptional cases. Case 2, I think could be changed to plumb through locations explicitly, but it's a bit annoying, so I didn't tackle it here. Case 1, I think can probably just be changed to consistently initialize, and if you want a proper line number, you need to put it in there explicitly. However, I didn't change that in this PR, because I think it could be slightly breaking, so should be pkgeval'd.
Keno
added a commit
that referenced
this pull request
Mar 22, 2024
I don't like `jl_filename`/`jl_lineno`. They are weird internal state, and they are also not thread safe, so if different threads are evaling different things at the same time, line numbers can get confused. This PR changes the core function `jl_toplevel_eval_flex` to keep track of its current file/line context on the stack, so at least there is no confusion within one call to this function. With this PR and #53797, the global `jl_filename`/`jl_lineno` are used for three purposes: 1. To initialize the filename/lineno used by lowering from `Core.eval`. 2. To give binding deprecation warnings. 3. For `jl_critical_error`. 4. By humans in the debugger. I think 3 and 4 are fine, they are exceptional cases. Case 2, I think could be changed to plumb through locations explicitly, but it's a bit annoying, so I didn't tackle it here. Case 1, I think can probably just be changed to consistently initialize, and if you want a proper line number, you need to put it in there explicitly. However, I didn't change that in this PR, because I think it could be slightly breaking, so should be pkgeval'd.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
These flisp accessor functions make use of the
jl_filenameandjl_linenoglobals. I was looking at removing these globals for unrelated reasons, when I saw that one of the primary uses was in these flisp accessor, which appear entirely unnecessary. They are only used to provide a default for an error message, but the place that puts the error message into a list to return to julia does actually already have access to this information, so there's no need for these to look at the globals.While we're at it, also add a test for this code path, which was otherwise unexercised in our test suite.