Clean up lowering API #58147
Merged
Clean up lowering API #58147
Conversation
This file contains hidden or 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
vtjnash
approved these changes
Apr 17, 2025
Member
vtjnash
left a comment
vtjnash
left a comment
There was a problem hiding this comment.
It looks like the stmt arg form was mainly a hack to change the meaning of an expression for a deprecation, which isn't necessarily applicable anymore
mlechu
force-pushed
the
lowering-api
branch
2 times, most recently
from
5dc7870 to
e994dba
Compare
topolarity
approved these changes
Apr 22, 2025
Member
topolarity
left a comment
topolarity
left a comment
There was a problem hiding this comment.
LGTM. Seems like a pretty straightforward (small) re-factor
unless @JeffBezanson has any feedback, I'll merge this today
and redirect all calls to lowering through it. `jl_fl_lower` is essentially a
renamed `jl_expand_with_loc_warn`, but change the structure of the return
value of its callee `jl-expand-to-thunk-warn` so we can ignore the warnings
if they aren't relevant.
`jl_expand_stmt`, `jl_expand_stmt_with_loc`, `jl_expand_with_loc`
This function has always been a convenience wrapper providing default arguments
to another lowering function. We can give it a more accurate name now that
there aren't five other wrappers we would also need to rename in a
consistent way.
This seems to be more accurate.
Member
|
Thanks @mlechu ! |
yuyichao
added a commit
to yuyichao/NLopt.jl
that referenced
this pull request
May 26, 2025
Broken by JuliaLang/julia#58147 . However, this has always been an error outside of a module block so the error is probably consistent
odow
pushed a commit
to jump-dev/NLopt.jl
that referenced
this pull request
May 26, 2025
Broken by JuliaLang/julia#58147 . However, this has always been an error outside of a module block so the error is probably consistent
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
This is preparatory work for experimental integration with JuliaLowering, which
happened to be separable into its own PR.
We currently have three lowering entry points doing slightly different things:
jl-expand-to-thunk,jl-expand-to-thunk-warn(for complaining about ambiguoussoft scope assignments during non-interactive use), and
jl-expand-to-thunk-stmt(which wraps the expression in a block returningnothing before lowering it) and a bunch of C wrappers on top of those (see red
nodes in the call graphs below).
In this PR:
jl_lower, which just calls out to lispfor now, but will probably function like
jl_parsedoes in a future PR.warnwith an extra parameterthree entry points
"expand"-prefixed functions to "lower"-prefixed ones (excluding macro
expansion functions, which are called as the first part of lowering).
Here's a call graph before this change, made by looking for callers
of each lowering entry point and tracing back one or more steps (expect
mistakes...). Macro expansion functions are mostly omitted for clarity.
Blue is scheme, red is ast.c, and green is toplevel.c.
After this change:
todo?
I'd like to see if we can eliminate thestmtboolean option fromjl_lowerand handle that another way; I'm just not sure it's worth the effort at the
moment. We only use it in one place in
jl_eval_module_expr. The originalcode path was added in lower top-level statements so that the front-end knows their values are unused #26304.