Simple comment formatting, unnecessary split

Good call. The intention was to split the documentation and the meta-level comment (explaining how this predicate came to be). But like you said it can be disconnected easily, so I'll merge the meta-level comment into the docstring first.

Addressed inc8c0770.

Likely want to test that this works for aint * const x:

voidf(int *const x) {    (*x)++;}intmain() {for (int i =0; i <10; ++i) {f(&i);        std::cout << i << std::endl;    }}

I believe what will happen is thatint * const x will be aDerivedType of typeSpecifiedType with a const specifier. ASpecifiedType is notinstanceof PointerType orinstanceof ReferenceType and so this predicate will not hold, even though the value ofi is modifiable withinf.

You may also have problems with typedefs, such astypedef int *int_ptr_t for the same reason.

The solution here I believe will be to call.getUnderlyingType(). Another option frequently used for this is.stripSpecifiers(). Each of these will remove the const and resolve the typedef. I think.stripSpecifiers() may remove the const inconst int*, though, which would make it unsuitable here.

You're right; the predicate does not catch this example. 🤔 I guess a clever use of one or more ofisDeeplyConst, orisDeeplyConstBelow will do the trick.

Forgetting to handle typedefs or meaningless consts is a very common bug. But you'll (mostly) get in the habit soon enough of always calling one of these four member predicates on theTypes you handle in your queries:

• getUnderlyingType()
• resolveTypedefs()
• stripSpecifiers()
• stripTopLevelSpecifiers()

Each one does subtly different things.

In this case, I believe the fix is to do:

exists(...,TypetargetType,DerivedTypestrippedType|    isAssignment(assignmentRhs, targetType, _)and    strippedType=targetType.stripTopLevelSpecifiers()    not strippedType.getBaseType().isConst() and(      strippedType instanceofPointerTypeor      strippedTypeinstanceofReferenceType)

The documentation forstripTopLevelSpecifiers says:

Get this type after any top-level specifiers and typedefs have been stripped.

For example, starting withconst i64* const, this predicate will returnconst i64*.

which is actually wrong, as it ignores the fact thati64 is aTypeDefType, so itactually will result inconst long long*. Which is what you want!

The TLDR of the other options:

• getUnderlyingType() -- resolvesTypdefTypes andDeclTypes, but won't drop the outer specifer inconst i64* const. Stops at the first non-TypedefType/non-DeclType.
• stripType() -- resolves all typedefs and decltypes and removes allconst/volatile specifiers recursively all the way down the type chain -- not what you want.
• resolveTypedefs -- resolves all typedefs and decltypes all the way down the type chain without removingconst orvolatile specifiers. That would handle typedefs but notint const *.

Note that these predicates can have no result. Only a limited set of types are in the database, and these operations just assume that the type you want is one of those types.resolveTypedefs is also bugged and doesn't recurse intoArrayType.

Thank you for the detailed breakdown of the related predicates. What I want to express here is definitely "The type we getafter we strip all the typedefs and the specifiers is const". I've come to believestripTopLevelSpecifiers is the one I should use, and swapped the portion with your suggestion.

I also patched an equivalent part inloopVariablePassedAsArgumentToNonConstReferenceParameter, in7d5f08b.

You may want to addnot assignmentRhs.isInUnevaluatedContext() for safety.

That would prevent reporting cases likesizeof(g(&i)) ordecltype(g(&i)).

Feel free to resolve this.

I mostly wanted to point this out because this is a common trap case in coding standards query writing. In C/C++ you can write any expression inside of asizeof() check, as well as other exprs likedecltype()/alignof()/constexpr()/requires()`, and these aren't evaluated.

So if you're looking at a rule that says something like, "Never passnullptr intostd::some_function(x)", then that's the kind of rule where we may want to be careful thatsizeof(std::some_function(nullptr)) isn't flagged, because it won't actually executestd::some_function().

In this case, I've convinced myself this isn't something we have to worry about in this case. Hopefully I'm not wrong about that! :)

template<typename IndexType, typename Observer>void process_with_observer(std::vector<int>& data) {    Observer observer;        for (IndexType i = 0; i < data.size(); ++i) {        // Determine observer's interface at compile time        using observer_result = decltype(observer(&i));                if constexpr (std::is_void_v<observer_result>) {            observer(&i);            data[i] = default_transform(data[i]);        } else {            auto metadata = observer(&i);            data[i] = complex_transform(data[i], metadata);        }    }}

Two missed cases here:

• Mixing signed/unsigned types, they may have the same size but they'll hold different ranges.
• The type and runtime value may lead to different conclusions.

I think you may be able to get away withupperBound(loopCounter) < upperBound(loopBound). That would handle signedness, constants (likex < 10ull), and dynamic ranges (likeunsigned long long bound = 10; ... x < bound).

Also almost forgot

Another trap case is that when doingupperBound(e) /lowerBound(e) you usually wantupperBound(e.getFullyConverted()). Because conversions one will change the bound.

A thought on simplifying these names.

Maybe instead of naming themloopVariablePassedAs... you can rename them topassedAsNonConstReference/passedAsNonConstPointer and remove theForStmt argument.

Then at the call sites you can change

loopVariablePassedAsArgumentToNonConstReferenceParameter(loop, va)// becomesexists(VariableAccess other |  passedAsNonConstReference(other) and  other.getVariable() = loop.getBound().(VariableAccess).getVariable() and  other.getEnclosingStmt().getParent*() = loop.getStmt())

I'm not sure this is a good idea because

• TLoopCounterIsTakenNonConstAddress and friends have to have an identical body, and repeating these lines three times will increase the verbosity.
• The predicatesloopVariablePassedAs... and the other one are highly specialized to this query alone, so it doesn't hurt much to keep them amalgamated (if that's a word!) and not break them down.

I wouldn't worry about this as extra verbosity because the extra code is easy to read. In general, I'd worry more about clear/concise naming.

If verbosity is an issue you can add helper methods toLegacyForLoopCondition

class LegacyForLoopCondition {  ...  predicate isBoundAccess(VariableAccess va) {    va.getVariable() = getBound().(VariableAccess).getVariable() and    other.getEnclosingStmt().getParent*() = loop.getStmt()  }  ...}...  exists(VariableAccess other    | passedAsNonConstReference(other) and loopCondition.isBoundAccess(other))

You can definitely keep it as is if you prefer, and you may think of a better way to abstract the duplicated code than theisBoundAccess() example here.

Definitely your call!