Skip to content

Conversation

MathiasVP
Copy link
Contributor

@MathiasVP MathiasVP commented Feb 15, 2024

This is a preparatory change to an upcoming PR I'm doing related to the "automatically block flow out of modelled functions" work. It won't really have any visible impact yet, so it should be a totally safe change.

The core of the change is that we were incorrectly calling getUnspecifiedType instead of getUnderlyingType when checking whether a parameter could be written to. Calling getUnspecifiedType is wrong since, in addition to resolve typedefs, it also strips off const specifiers.

Instead, we should be calling getUnderlyingType since that only resolves typedefs.

DCA looks uneventful (as expected).

@MathiasVP MathiasVP requested a review from a team as a code owner February 15, 2024 13:11
@github-actions github-actions bot added the C++ label Feb 15, 2024
@MathiasVP MathiasVP added the no-change-note-required This PR does not need a change note label Feb 15, 2024
@@ -452,7 +452,7 @@ private module IsModifiableAtImpl {
private predicate impl(CppType cppType, int indirectionIndex) {
exists(Type pointerType, Type base |
isUnderlyingIndirectionType(pointerType) and
cppType.hasUnderlyingType(pointerType, _) and
cppType.hasUnderlyingType(pointerType, false) and
Copy link
Contributor Author

@MathiasVP MathiasVP Feb 15, 2024

Choose a reason for hiding this comment

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

This was a small drive-by optimization I did: Since we only ever care about non-glvalue types in this predicate there's no need to get both the glvalue and the prvalue version of the type as we compute this pipeline

exists(CppType cppType |
cppType.hasUnspecifiedType(unspecified, _) and
isModifiableAt(cppType, indirectionIndex + 1)
cppType.hasUnderlyingType(underlying, false) and
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The false here is a similar drive-by optimization just like here

@MathiasVP MathiasVP merged commit c19ed4c into github:main Feb 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C++ no-change-note-required This PR does not need a change note
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants