vrnithinkumar added inline comments.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:137
+ const auto *RecordDecl = MethodDecl->getParent();
+ if (!RecordDecl || !RecordDecl->getDeclContext()->isStdNamespace())
+ return InnerType;
----------------
xazax.hun wrote:
> I'd rather use `Decl::isInStdNamespace` instead of querying the DeclContext
> of the decl. The former is more robust with inline namespaces.
Changed to use `Decl::isInStdNamespace`
================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:141
+ const auto *TSD = dyn_cast<ClassTemplateSpecializationDecl>(RecordDecl);
+ if (TSD) {
+ auto TemplateArgs = TSD->getTemplateArgs().asArray();
----------------
xazax.hun wrote:
> Inverting this condition would reduce the indentation in the rest of the
> function.
inverted the if condition
================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:145
+ TemplateArgs.size() > 0 &&
+ "Smart pointer should have specialized with atleast one template
type");
+ auto InnerValueType = TemplateArgs[0].getAsType();
----------------
NoQ wrote:
> That's pretty fundamental, right? If it's a specialization, it must have
> something specialized. It isn't specific to unique pointers, right?
>
> Because unique pointers aren't special; technically anybody can define an
> arbitrary class with name `std::unique_ptr` and any properties they'd like.
> It's going to be undefined behavior according to the standard (because
> namespace `std` is explicitly reserved for the standard library) but if the
> compiler *crashes* it'll still be our fault.
>
>
added a check for `TemplateArgs.size() == 0` before accessing the first
Template argument.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:147
+ auto InnerValueType = TemplateArgs[0].getAsType();
+ InnerType =
+ C.getASTContext().getPointerType(InnerValueType.getCanonicalType());
----------------
xazax.hun wrote:
> You could return the real inner type here and replace all other returns with
> `return {};` making the code a bit cleaner.
Updated to return with {}
================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:404
+void SmartPtrModeling::handleBoolOperation(const CallEvent &Call,
+ CheckerContext &C) const {
----------------
vsavchenko wrote:
> I suggest `BoolConversion`
yeah thats sounds better.
Changed.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:413
+ if (InnerPointVal) {
+ bool IsInnerPtrNull = InnerPointVal->isZeroConstant();
+ State = State->BindExpr(CallExpr, C.getLocationContext(),
----------------
xazax.hun wrote:
> 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.
Fixed as you suggested in the below comments
================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:432
+ return;
+ } else {
+ // In case of inner pointer SVal is not available we create
----------------
xazax.hun wrote:
> 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.
Made the changes as suggested above
================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:448
+
+ if (NullState) {
+ auto NullVal = C.getSValBuilder().makeNull();
----------------
xazax.hun wrote:
> 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.
Refactored the method as suggested.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:458-460
+ OS << "Assuming smart pointer ";
+ ThisRegion->printPretty(OS);
+ OS << " is null";
----------------
NoQ wrote:
> Wait, what happens when the region can't be pretty-printed? Does it print two
> spaces between "pointer" and "is"?
Yes.
Created `checkAndPrettyPrintRegion` to check if the region can be pretty
printed or not to avoid two spaces.
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
