-
Notifications
You must be signed in to change notification settings - Fork 280
transpile: Extend lifetime of compound literals whose address is taken #1407
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
base: master
Are you sure you want to change the base?
Conversation
ec33fb8
to
0d75d02
Compare
0d75d02
to
bdcf6e5
Compare
011c412
to
433f73a
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.
LGTM. Thanks! Just a couple of comment suggestions.
c30233e
to
398d073
Compare
I added extra tests to ensure that compound literals in macros don't undergo the same treatment as normal. Because they can be borrowed as statics, same as in #1379. However, I notice that the macro isn't used in the |
caab1d1
to
2188649
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.
I added extra tests to ensure that compound literals in macros don't undergo the same treatment as normal. Because they can be borrowed as statics, same as in #1379. However, I notice that the macro isn't used in the
macro_int_array_ptr
andmacro_char_array_ptr
cases, it's expanded to its definition and then given a fresh variable. Any idea why the macro is bypassed here?
If you set log_level: log::LevelFilter::Info
in fn config
in c2rust-transpile/tests/snapshots.rs
, you can see the error:
info: Could not expand macro INT_ARRAY: Macro expansion is not a pure expression
info: Could not expand macro CHAR_ARRAY: Macro expansion is not a pure expression
So it seems fn to_pure_expr
is failing because there are statements. I think you can also add || ctx.is_const
to fix it.
2188649
to
4a62153
Compare
a57aec9
to
b87be63
Compare
This should be ready to go now. |
b87be63
to
f32a53d
Compare
f32a53d
to
7a15b80
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.
One of the snapshot lines was off in the first commit, but I fixed it, so this LGTM now. Thanks again!
I've only looked at fixing it in function context, as it's not possible to add statements to static initializers ("Expected no side-effects in static initializer" panic). But I'm not sure if the issue exists for statics anyway, because Rust is forced to extend the lifetime of the temporary to static in such a context.
There is also an issue with how this line compiles in the snapshot test:
It gets translated into an array in which the first element is the translated string, and then five zero integer literals. That's clearly broken and doesn't compile in Rust, so I commented out that line. Since my fix doesn't change how statics are handled, I believe that's an existing bug in c2rust that needs its own fix.