Skip to content

Conversation

ahmedshakill
Copy link
Contributor

Implements #1812

Copy link

github-actions bot commented Aug 24, 2025

✅ With the latest revision this PR passed the C/C++ code formatter.

Copy link
Collaborator

@tommymcm tommymcm left a comment

Choose a reason for hiding this comment

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

lgtm, some nits

@ahmedshakill ahmedshakill requested a review from tommymcm August 25, 2025 06:16
Copy link
Collaborator

@tommymcm tommymcm left a comment

Choose a reason for hiding this comment

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

CIRGen lgtm
Some more changes for the test to format it correctly and make it more readable.

Copy link
Collaborator

@tommymcm tommymcm left a comment

Choose a reason for hiding this comment

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

lgtm

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.

LGTM after minor comment

@ahmedshakill
Copy link
Contributor Author

Qualified explicit destructor call has different behavior compared to Non-Qualified destructor call.

qualifying destructor

Qualified explicit destructor call is already handled in the CIR Codegen. Only Unqualified call needs to be handled.

CIR qualifying destructor

@tommymcm
Copy link
Collaborator

Qualified explicit destructor call has different behavior compared to Non-Qualified destructor call.

qualifying destructor Qualified explicit destructor call is already handled in the CIR Codegen. Only Unqualified call needs to be handled. CIR qualifying destructor

Sounds good, is there any issue here or did you just want to clarify?

@ahmedshakill
Copy link
Contributor Author

@tommymcm Just wanted to clarify.

Copy link
Collaborator

@tommymcm tommymcm left a comment

Choose a reason for hiding this comment

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

Needs some more info in the tests, once that's done I think it will all lgtm

@ahmedshakill ahmedshakill requested a review from tommymcm August 29, 2025 19:45
Copy link
Collaborator

@tommymcm tommymcm left a comment

Choose a reason for hiding this comment

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

Missed the this store in the CIR test (sorry for not catching that earlier)
One nit in the OGCG test (unneeded CHECK)

Copy link
Collaborator

@tommymcm tommymcm left a comment

Choose a reason for hiding this comment

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

lgtm

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.

4 participants