NoQ added a comment.
This looks good at a glance to me! I only have minor nits left.
It looks like we still need the option to be in place even though we moved the
warnings into a separate checker, so that not to accidentally behave worse than
with conservative modeling, but it becomes much less important.
================
Comment at:
clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h:792-794
+ OverloadedOperatorKind getOverloadedOperator() const {
+ return getOriginExpr()->getOperator();
+ }
----------------
Nice!
================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrChecker.cpp:41
+
+ bool checkDeferenceOps(const CallEvent &Call, CheckerContext &C) const;
+};
----------------
Looks like dead code.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrChecker.cpp:72
+ NullDereferenceBugType, "Dereference of null smart pointer", ErrNode);
+ R->addRange(Call.getSourceRange());
+ C.emitReport(std::move(R));
----------------
This is probably unnecessary. The default `SourceRange` highlighted in
path-sensitive report is the error node's statement and it should be exactly
the same.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:81
CheckerContext &C) const {
- if (!isNullAfterMoveMethod(Call))
+ if (isNullAfterMoveMethod(Call)) {
+ ProgramStateRef State = C.getState();
----------------
vrnithinkumar wrote:
> xazax.hun wrote:
> > Don't we want this to be also protected by the `isStdSmartPointerClass`
> > check?
> added `isStdSmartPointerClass` check in the beginning.
Uh-oh, i'm shocked this wasn't in place before. Maybe we should do some code
review or something. What idiot wrote this code anyway? Wait, it was me.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:50
+ void handleSwap(const CallEvent &Call, CheckerContext &C) const;
+ bool checkDeferenceOps(const CallEvent &Call, CheckerContext &C) const;
+
----------------
Looks like dead code.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:67-80
+bool isStdSmartPtrCall(const CallEvent &Call) {
+ const auto *MethodDecl = dyn_cast_or_null<CXXMethodDecl>(Call.getDecl());
+ if (!MethodDecl || !MethodDecl->getParent())
+ return false;
+
+ const auto *RecordDecl = MethodDecl->getParent();
+ if (!RecordDecl || !RecordDecl->getDeclContext()->isStdNamespace())
----------------
Ok, so the normal solution to this problem is to make this logic a part of your
`CallDescriptionMap`:
```lang=c++
CallDescriptionMap<SmartPtrMethodHandlerFn> SmartPtrMethodHandlers{
{{"std", "unique_ptr", "reset"}, &SmartPtrModeling::handleReset},
{{"std", "unique_ptr", "release"}, &SmartPtrModeling::handleRelease},
{{"std", "unique_ptr", "swap", 1}, &SmartPtrModeling::handleSwap},
{{"std", "shared_ptr", "reset"}, &SmartPtrModeling::handleReset},
{{"std", "shared_ptr", "release"}, &SmartPtrModeling::handleRelease},
{{"std", "shared_ptr", "swap", 1}, &SmartPtrModeling::handleSwap},
{{"std", "weak_ptr", "reset"}, &SmartPtrModeling::handleReset},
{{"std", "weak_ptr", "release"}, &SmartPtrModeling::handleRelease},
{{"std", "weak_ptr", "swap", 1}, &SmartPtrModeling::handleSwap},
};
```
It looks like you're not doing this because `CallDescriptionMap` doesn't
support operator calls yet. In fact, it looks like it doesn't support them
because i'm not doing my homework by reviewing D81059. I just tried to catch up
on it.
The other potential reason not to use `CallDescriptionMap` would be that you'll
have to duplicate the list of methods for every smart pointer class you want to
support. I don't think it's necessarily bad though, because different classes
may in fact require different handling.
The downside of your solution, though, is that you're manually implementing the
name matching logic that has been already implemented for you in
`CallDescriptionMap`. And the reason we made `CallDescriptionMap` was because
we really really wanted to re-use this logic because it's surprisingly
difficult to implement correctly. One potential problem i see with your
implementation is that you don't account for inline namespaces such as [[
https://github.com/llvm/llvm-project/blob/master/clang/unittests/StaticAnalyzer/CallDescriptionTest.cpp#L127
| libcxx's `__1` ]]. `CallDescriptionMap` knows about these things but they're
almost impossible to discover independently when you're matching names by hand.
Let's leave this code as-is for now but try to get rid of this function as soon
as possible (i.e., when D81059 lands).
================
Comment at: clang/test/Analysis/smart-ptr.cpp:5
// RUN: -std=c++11 -verify %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=core\
+// RUN: -analyzer-checker alpha.cplusplus.SmartPtr\
----------------
This second run-line no longer tests the option. The checker is disabled, of
course there won't be any diagnostics. I think we should remove the second
run-line now and i don't have much better tests in mind that we won't
eventually remove as we write more code.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D81315/new/
https://reviews.llvm.org/D81315
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
