vrnithinkumar added inline comments.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrChecker.cpp:41
+
+ bool checkDeferenceOps(const CallEvent &Call, CheckerContext &C) const;
+};
----------------
NoQ wrote:
> Looks like dead code.
Thanks!
removed.
================
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));
----------------
NoQ wrote:
> 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.
Removed.
================
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())
----------------
NoQ wrote:
> 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).
Thanks for the details.
If I recall correctly, I also found that `CallDescriptionMap` was not
supporting constructors also.
I am not sure why.
Is it because constructor name is a special name?
================
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;
+
----------------
NoQ wrote:
> Looks like dead code.
Thanks!
removed.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:81
CheckerContext &C) const {
- if (!isNullAfterMoveMethod(Call))
+ if (isNullAfterMoveMethod(Call)) {
+ ProgramStateRef State = C.getState();
----------------
NoQ wrote:
> 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/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\
----------------
NoQ wrote:
> 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.
Okay thanks
removed the second line.
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
