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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
