xazax.hun added inline comments.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:35
bool isNullAfterMoveMethod(const CallEvent &Call) const;
+ BugType NullDereferenceBugType{this, "Null-smartPtr-deref",
+ "C++ smart pointer"};
----------------
Nit: We do not use hypens/dashes in diagnostic names.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:53
+ // Set of STL smart pointer class which we are trying to model.
+ const llvm::StringSet<> StdSmartPtrs = {
+ "shared_ptr",
----------------
It might be just a personal preference but I tend to put these at the top of
the file for easier discoverability. Feel free to ignore this comment unless
other reviewers have the same preference as me.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:81
CheckerContext &C) const {
- if (!isNullAfterMoveMethod(Call))
+ if (isNullAfterMoveMethod(Call)) {
+ ProgramStateRef State = C.getState();
----------------
Don't we want this to be also protected by the `isStdSmartPointerClass` check?
================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:115
+ const auto *MethodDecl = dyn_cast<CXXMethodDecl>(Call.getDecl());
+ if (!MethodDecl || !isStdSmartPointerClass(MethodDecl->getParent()))
+ return;
----------------
I wonder if making `isStdSmartPointerClass` operate on `CallEvent` would
simllify the call sites of this function.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:137
+ if (IsRegDead) {
+ State = State->remove<TrackedRegionMap>(Region);
+ }
----------------
In LLVM we often omit braces for small single statement branches. Also, Artem
mentioned that we could interact with the malloc checker. Maybe it is worth
considering to notify the malloc checker to mark the appropriate region as
deallocated? This would help find double release errors, avoid spurious leak
errors and so on.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:155
+
+ if (MethodDecl && MethodDecl->isOverloadedOperator()) {
+ OverloadedOperatorKind OOK = MethodDecl->getOverloadedOperator();
----------------
Early returns can decrease the indentation.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:213
+ if (NumArgs == 0) {
+ auto NullSVal = C.getSValBuilder().makeNull();
+ State = State->set<TrackedRegionMap>(ThisValRegion, NullSVal);
----------------
I wonder if we should use `makeNullWithType`. @NoQ what do you think? I am
always confused when should we prefer one over the other.
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
