Conversation
kkysen
left a comment
There was a problem hiding this comment.
@fw-immunant, same as #1334 (review).
b9834b2 to
3eec9fa
Compare
1e48cb0 to
14ca587
Compare
…ranslate-const-macros conservative` Some operations, such as ptr arithmetic, are `unsafe` and can be done in const macros. So as an initially overly conservative implementation, just make all `const`s use `unsafe` blocks in case `unsafe` operations are used. This is what we already do for `fn`s, for example, even if all of the operations in a `fn` are safe.
14ca587 to
3635b4e
Compare
fw-immunant
left a comment
There was a problem hiding this comment.
This makes our translation of a simple #define FOO 5 less idiomatic, which seems contrary to the goal of providing fully idiomatic translation for the simplest cases of #defines. I've prototyped an alternate strategy to adding unsafe to const macro translations where necessary in #1339.
kkysen
left a comment
There was a problem hiding this comment.
This makes our translation of a simple
#define FOO 5less idiomatic, which seems contrary to the goal of providing fully idiomatic translation for the simplest cases of#defines.
I'm not sure if the goal was to provide fully idiomatic translation for the simplest #defines. We've never strived for c2rust transpile emitting fully idiomatic code, but rather, as idiomatic as reasonably possible. I'd like to merge this first as the simplest way to translate more const macros, and then we can work on things like #1339 on top of this (I'll leave more comments there). You made #1339 point at #1306, which points at #1335 (this PR), which seems like the right ordering to me, so if that's what you intended, then can we merge this PR now?
That's what I understood from the phrase "low-hanging fruit"--the simple cases where we can get the exact right answer (fully idiomatic translation) without necessarily having a solution that can handle the full generality of inputs. Maybe @thedataking could clarify what our priorities are here. I'd rather not merge this because it makes translation of simple uses of #1306 is approved and can merge into this branch whenever you like. |
fw-immunant
left a comment
There was a problem hiding this comment.
We discussed the various const macro changes in flight and decided to land this and follow-ups rather than trying to stage everything outside master.
--translate-const-macros conservativeto a lot moreCExprKinds #1306.Some operations, such as ptr arithmetic, are
unsafeand can be done in const macros. So as an initially overly conservative implementation, just make allconsts useunsafeblocks in caseunsafeoperations are used. This is what we already do forfns, for example, even if all of the operations in afnare safe. We can improve this, but for now, this works and is correct.