-
Notifications
You must be signed in to change notification settings - Fork 279
transpile: separate top-level decl converting and emitting #1371
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
Interesting, the testsuite failure shows us rewriting an expansion of the #define NTLMv2_BLOB_LEN (44 -16 + ntlm->target_info_len + 4) |
793e6c7
to
98e803b
Compare
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.
Interesting, the testsuite failure shows us rewriting an expansion of the
NTLMv2_BLOB_LEN
macro, but we shouldn't be treating this macro as successfully converted to aconst
because it references a free (at the definition site) variable
Yeah, and I'm not sure why changing the order affects this. Still looking into it...
98e803b
to
3620fdd
Compare
Determining which declarations have been declared within the scope of the const macro expr vs. which are out-of-scope of the const macro is non-trivial, so for now, we don't allow const macros referencing any declarations. This should fix some of the issues surfaced in #1371 when the order of top-level decl being converted was changed.
3620fdd
to
850118b
Compare
…tted code) This sets things up to iterate through different orders for each.
…efore they're inserted This should ensure that those two steps are properly separated.
850118b
to
b85ef7b
Compare
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.
Interesting, the testsuite failure shows us rewriting an expansion of the
NTLMv2_BLOB_LEN
macro, but we shouldn't be treating this macro as successfully converted to aconst
because it references a free (at the definition site) variable:#define NTLMv2_BLOB_LEN (44 -16 + ntlm->target_info_len + 4)
This should be skipped by #1387, so merging this now.
Right now, we sort the top-level decls once by their source order (although we do this largely backwards, a separate issue) before we convert them and then emit them (insert in the code). This means that changing the conversion order also changes the order that they're emitted in, which we don't want to do, so this separates the two steps.