[PATCH] D86027: [analyzer] Add bool operator modeling for unque_ptr

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun requested changes to this revision.
xazax.hun added a comment.
This revision now requires changes to proceed.

Please add a test case, where the unique_ptr is initialized from a pointer parameter that has no assumptions. I think that case is not handled correctly.



================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:137
+  const auto *RecordDecl = MethodDecl->getParent();
+  if (!RecordDecl || !RecordDecl->getDeclContext()->isStdNamespace())
+    return InnerType;
----------------
I'd rather use `Decl::isInStdNamespace` instead of querying the DeclContext of the decl. The former is more robust with inline namespaces. 


================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:141
+  const auto *TSD = dyn_cast<ClassTemplateSpecializationDecl>(RecordDecl);
+  if (TSD) {
+    auto TemplateArgs = TSD->getTemplateArgs().asArray();
----------------
Inverting this condition would reduce the indentation in the rest of the function.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:147
+    auto InnerValueType = TemplateArgs[0].getAsType();
+    InnerType =
+        C.getASTContext().getPointerType(InnerValueType.getCanonicalType());
----------------
You could return the real inner type here and replace all other returns with `return {};` making the code a bit cleaner.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:413
+  if (InnerPointVal) {
+    bool IsInnerPtrNull = InnerPointVal->isZeroConstant();
+    State = State->BindExpr(CallExpr, C.getLocationContext(),
----------------
Is this actually correct? What if the InnerPtr is an unconstrained symbol. In that case, it is not a zero constant so we will assume that it is constrained to non-zero. As far as I understand.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:432
+    return;
+  } else {
+    // In case of inner pointer SVal is not available we create
----------------
I'd do it the other way around as we discussed during the call.
* Move the task of conjuring a new symbol to the beginning of the method.
* Start by calling this function at the beginning of modeling operator bool.
* The rest of the function could assume that there always is a symbol. It could be constrained to be non-null, it could be the zero constant, or it could be a completely unconstrained symbol. The latter will not work as expected with your current implementation, see my comment above.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:448
+
+    if (NullState) {
+      auto NullVal = C.getSValBuilder().makeNull();
----------------
NoQ wrote:
> There's no need to check. You just conjured a brand new symbol out of thin air; you can be sure that it's completely unconstrained and both states are feasible. You can instead `assert()` that they're both feasible.
I think instead of removing this check, this method should be reworked. I think it might have some bugs, see my comment at the beginning of this method.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D86027/new/

https://reviews.llvm.org/D86027