[PATCH] D119601: [analyzer] Refactor makeNull to makeNullWithWidth (NFC)

[Balázs Benics via Phabricator via cfe-commits]

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