Skip to content

Conversation

bruteforceboy
Copy link
Contributor

This PR fixes Issue#1647.

It just takes the implementation from emitRethrow and extends the same logic to emitThrow.

The only nitpick about the fix is same as before - we have this redundant ScopeOp which acts as a placeholder, so there are some redundant yield blocks in some cases. Aside that, I believe this fix is okay for now.

I have added the tests from the issue to confirm everything works as intended.

cc: @mmha, @bcardosolopes.

Copy link
Collaborator

@mmha mmha left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. My only request is one more test case:

int ternary_throw(bool condition, int x) {
    return condition ? throw x : x;
}

This tests both the awkward ternary expression case and should make the newly created block take a new argument

@bruteforceboy
Copy link
Contributor Author

LGTM. My only request is one more test case:

int ternary_throw(bool condition, int x) {
    return condition ? throw x : x;
}

This tests both the awkward ternary expression case and should make the newly created block take a new argument

Sorry for the late reply. I have updated the PR to handle this case. Changes made:

  • To get the terminator when building the TernaryOp, we now get the terminator of the last block in the true region instead, since the ThrowOp now splits into multiple blocks.
  • Got rid of Lexical scopes [1] [2] in the TernaryOp builder for the AbstractConditionalOperator. I have compared with the original CodeGen and it seems to be the way to go. I have also attached the LLVM output in the tests for verification.

Copy link
Member

@bcardosolopes bcardosolopes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome! LGTM

@bcardosolopes bcardosolopes merged commit 94402e2 into llvm:main Jun 4, 2025
9 checks passed
terapines-osc-cir pushed a commit to Terapines/clangir that referenced this pull request Sep 2, 2025
This PR fixes [Issue#1647](llvm#1647).

It just takes the implementation from
[`emitRethrow`](https://github.com/llvm/clangir/blob/105d898b9898d224f0baca4b161a84bdcf817617/clang/lib/CIR/CodeGen/CIRGenItaniumCXXABI.cpp#L2273C1-L2276C77)
and extends the same logic to `emitThrow`.

The only nitpick about the fix is same as before - we have this
[redundant
ScopeOp](https://github.com/llvm/clangir/blob/105d898b9898d224f0baca4b161a84bdcf817617/clang/lib/CIR/CodeGen/CIRGenItaniumCXXABI.cpp#L2298C1-L2303C37)
which acts as a placeholder, so there are some redundant yield blocks in
some cases. Aside that, I believe this fix is okay for now.

I have added the tests from the issue to confirm everything works as
intended.

cc: @mmha, @bcardosolopes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Throw Expressions should (probably) insert a new block

3 participants