Conversation
|
Thanks for the PR. Before I review this in detail, two questions:
Let me know once this is ready for review -- I'm assuming draft means draft. Thanks! |
|
It is AI co-authored (the commit message is tagged as such). I have been testing this in EverParse and it seems to work well. I have a couple of small patches coming and then I'll mark it ready for review. Thanks for the heads-up on including the test ... will do. |
32a8e8d to
78b5c99
Compare
|
ping @protz Would be great to have your feedback and get this to a mergeable state ASAP. We have some clients waiting on the feature. |
|
Thanks for the ping. I am at a workshop until tomorrow but expect to have time to review this before the end of the week. Thanks! |
protz
left a comment
There was a problem hiding this comment.
High-level points.
- I think it this transformation would be easier on CStar instead of C11. C11 is just an abstract syntax tree ready for pretty-printing. I expected this to operate over CStar because 1) we have no transformations currently operating on C11, because it's very low-level and hard to work with, 2) there are already helpers that exist on CStar, such as
vars_ofand 3) it would save you the contortions you're doing inreplace_identwhere you have to unpeel the spec-and-decl in a convoluted way (see point 1). Also,returns are already explicit in CStar so it sounds like you'd be better off there, and I don't think it's a big deal to addLabelandGototo CStar for such purposes (it's not type-checked). - Why do multiple returns imply early return? (comment on
has_early_return):void f() { if (true) return; else return; }does not have early returns in my opinion, because both are in terminal position (they do not happen before we reach the end of the function). Perhaps this is a terminology thing, or perhaps the check needs to be updated. Can you clarify? I expectedhas_early_returnwould take an extra parameter that indicates whether you're in terminal position, and throwFoundwhen hitting aReturnnode. - It might be that you can achieve better code quality by operating on
CStar. I notice that you rewriteReturn einto{ ret_var = e; goto exit }with an additionalC.Compound. It feels like it'd be easier to writeflatten_compoundson CStar than C11. - I would error out if a void function returns a value (line 120). Actually, I'd be curious to see what
let f (x: unit) = if true then x else ()-- I suspect unit-to-void elimination will kick in, but a test case would be helpful - Another instance of point 3) above: I don't think it's super clear what
is_void_returndoes on the C11 syntactic structure, and this function would be crystal clear in CStar -- also you would avoid matching on bool vs BOOLEAN, etc. - Your zero-initializer can be
{ 0 }for all types. This works:
int main() {
_Bool x = { 0 };
return x;
}
- Would it make sense to have your test be a pulse test? It would then automatically track the generated C. (And I don't think it has to use pulse.)
Requested changes: switch the pass to operate over CStar + answers to questions above. Thank you!
|
Thanks! A quick response to this part:
This was the initial version. I then had it changed to use a type-specific initializer, as this is more idiomatic in the context where we are using it. Would you be okay with retaining a type-specific initializer? |
Adds a -goto_for_early_return CLI option. When enabled, functions with early returns are transformed to use goto-based control flow: - A result variable is allocated at the top (for non-void functions) - Every early return is replaced by assignment + goto - A label and final return are appended at the end The pass operates on the CStar AST (before lowering to C11) for simpler type access, natural block flattening, and use of existing helpers. Detection uses a position-aware walk that distinguishes terminal returns (in tail position) from early returns (in non-terminal position). Includes tests covering int32, uint32, uint64, int64, bool, void, struct, name collision avoidance, no-early-return passthrough, and unit/void interaction. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
78b5c99 to
e67ac84
Compare
Adds a new -goto_for_early_return CLI option that rewrites functions
containing early returns to use goto-based control flow. When enabled,
functions with non-tail return statements are transformed so that:
initializer (e.g. 0 for int32_t, 0U for uint32_t, NULL for pointers;
omitted for void functions).
issues with goto.
Fresh name generation avoids collisions with existing identifiers.
This is useful for targets that do not support early returns or where
goto-based control flow is preferred (e.g., certain embedded C dialects).
Changes:
Co-authored-by: Copilot 223556219+Copilot@users.noreply.github.com