Skip to content

C++: Handle block assignments #10031

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

Merged
merged 8 commits into from
Aug 15, 2022
Merged

C++: Handle block assignments #10031

merged 8 commits into from
Aug 15, 2022

Conversation

jketema
Copy link
Contributor

@jketema jketema commented Aug 11, 2022

Block assignments may occur as part of default copy/move constructors and assignment operators that are generated by the frontend. The assignments operate like memcpy, where the type of the source is used determine the size of what needs to be copied.

@github-actions github-actions bot added the C++ label Aug 11, 2022
@jketema jketema added the depends on internal PR This PR should only be merged in sync with an internal Semmle PR label Aug 14, 2022
@jketema jketema marked this pull request as ready for review August 15, 2022 06:27
@jketema jketema requested a review from a team as a code owner August 15, 2022 06:27
Copy link
Contributor

@MathiasVP MathiasVP left a comment

Choose a reason for hiding this comment

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

LGTM! I only have a small comment nitpick. Feel free to fix that up in a later PR to avoid an unnecessary submodule dance.

}

final override Instruction getFirstInstruction() {
// The operand evaluation order should since block assignments behave like memcpy.
Copy link
Contributor

Choose a reason for hiding this comment

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

This comment reads a bit weird. Did you mean "[...] should be left-to-right [...]"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Woops:

Suggested change
// The operand evaluation order should since block assignments behave like memcpy.
// The operand evaluation order should not matter since block assignments behave like memcpy.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Does that make more sense?

Copy link
Contributor

Choose a reason for hiding this comment

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

That makes a lot of sense, yes!

@@ -1450,8 +1450,6 @@ class TranslatedAssignExpr extends TranslatedNonConstantExpr {
result = this.getLeftOperand().getResult()
}

abstract Instruction getStoredValue();
Copy link
Contributor

Choose a reason for hiding this comment

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

Wat. This was just an unused abstract member predicate lying around? Well spotted!

@MathiasVP MathiasVP merged commit dfde571 into github:main Aug 15, 2022
@jketema jketema deleted the block-assign branch August 15, 2022 09:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C++ depends on internal PR This PR should only be merged in sync with an internal Semmle PR documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants