steakhal added inline comments.
================ Comment at: clang/include/clang/StaticAnalyzer/Core/PathSensitive/SValBuilder.h:368 Loc makeNullWithType(QualType type) { - return loc::ConcreteInt(BasicVals.getZeroWithTypeSize(type)); - } - - Loc makeNull() { - return loc::ConcreteInt(BasicVals.getZeroWithPtrWidth()); + assert((type->isPointerType() || type->isObjCObjectPointerType() || + type->isBlockPointerType() || type->isNullPtrType() || ---------------- Add a comment that `isAnyPointerType()` won't work here. ================ Comment at: clang/include/clang/StaticAnalyzer/Core/PathSensitive/SValBuilder.h:371 + type->isReferenceType()) && + "makeNullWithType must use pointer type"); + QualType nullType = type; ---------------- But you also let reference types. ================ Comment at: clang/include/clang/StaticAnalyzer/Core/PathSensitive/SValBuilder.h:372 - Loc makeNull() { - return loc::ConcreteInt(BasicVals.getZeroWithPtrWidth()); } ---------------- steakhal wrote: > You should also remove the `BasicValueFactor::getZeroWithPtrWidth()` along with `BasicValueFactor::getIntWithPtrWidth()` since those suffer from the same issue. Feel free to do that in a separate patch. ================ Comment at: clang/include/clang/StaticAnalyzer/Core/PathSensitive/SValBuilder.h:372-376 + QualType nullType = type; + if (type->isReferenceType()) { + nullType = Context.getPointerType(type->getPointeeType()); + } + return loc::ConcreteInt(BasicVals.getZeroWithTypeSize(nullType)); ---------------- ================ Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:123 +static QualType getTemplateSpecializationArgType(CheckerContext &C, + const CallEvent &Call, ---------------- You don't need the whole `CheckerContext`. Pass the `ASTContext` instead. I would also recommend using `Idx` instead of `ndx` ================ Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:133 + assert(T.getKind() == TemplateArgument::Type); + QualType Ty = C.getASTContext().getPointerType(T.getAsType()); + return Ty; ---------------- You should just return it. Oh wait, you also wrap it inside a pointer. The name of the function does not reflect this. Either rename it or hoist this to the callsite. ================ Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:402 + auto NullVal = C.getSValBuilder().makeNullWithType( + CC->getCXXThisVal().getType(C.getASTContext())); State = State->set<TrackedRegionMap>(ThisRegion, NullVal); ---------------- You should use the `CC->getDecl()->getThisType()` instead. ================ Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:660 + auto ValueToUpdate = + C.getSValBuilder().makeNullWithType(Call.getResultType()); State = State->set<TrackedRegionMap>(ThisRegion, ValueToUpdate); ---------------- Similarly to the previous one, use `getThisType()`. ================ Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:764 return false; - auto NullVal = C.getSValBuilder().makeNull(); + auto NullVal = C.getSValBuilder().makeNullWithType(Call.getResultType()); State = State->set<TrackedRegionMap>(ThisRegion, NullVal); ---------------- Same here. ================ Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:799 + auto NullVal = C.getSValBuilder().makeNullWithType( + getTemplateSpecializationArgType(C, Call, 0)); State = State->set<TrackedRegionMap>(OtherSmartPtrRegion, NullVal); ---------------- At first glance, all callsites have access to the `getThisType()`, so we shouldn't dig this up from the specialization decl. ================ Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:885 - auto NullVal = C.getSValBuilder().makeNull(); + auto NullVal = C.getSValBuilder().makeNullWithType(CallExpr->getType()); // Explicitly tracking the region as null. ---------------- NoQ wrote: > NoQ wrote: > > I suspect that this type is going to be `bool` which is probably not what > > you want. > I think it makes sense to add an assertion in `makeNullWithType()` to protect > against such cases. This will probably also allow you to cover this with a > test case. You probably never want to use `Call.getResultType()` in this file for this context. Something like this would be more appropriate: `cast<CXXMethodDecl>(Call.getDecl())->getThisType()` Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D119601/new/ https://reviews.llvm.org/D119601 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits