[PATCH] D97183: [analyzer] Add NoteTag for smart-ptr get()

[Valeriy Savchenko via Phabricator via cfe-commits]

vsavchenko added inline comments.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtr.h:57
+
+  void visitIfStmt(const ExplodedNode *Node, const ProgramStateRef State,
+                   BugReporterContext &BRC, const Stmt *S,
----------------
IMO, these three `visit` functions are a bit confusing in their names. I guess, my expectation for a group of methods that have very similar names is that they have similar signatures. And here `visitIfStmt` doesn't return diagnostic piece as opposed to two other methods.
We are not bound here to name everything `visitABC` since these methods are not part of the visitor interface.
My suggestion here is to actually keep `visitIfStmt` because that's what you actually do here, you simply **visit** it. Maybe change the return type to bool (signifying that it was indeed an if statement), so we don't check for other possibilities.
As for other two functions, I'd prefer something like `emitNoteForAssignment` and `emitNoteForInitialization` to keep the naming pattern going.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtr.h:37
+
+class GetNoteVisitor : public BugReporterVisitor {
+private:
----------------
vsavchenko wrote:
> Sorry for picking on it again, but is it visiting "get notes"? Otherwise, it is just a placeholder name that doesn't tell the reader what this class is actually for.
> Also, is there a reason why it is declared in the header file and couldn't be a part of `SmartPtrChecker.cpp` instead?
This comment is marked as "Done", but there is no code change nor justification.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtr.h:36
+
+class FindWhereConstrained : public BugReporterVisitor {
+private:
----------------
vsavchenko wrote:
> nit: class name should be a noun (functions and methods are verbs)
It is again some sort of verb. I don't know how to read it except for "emit note on get"-visitor. What about something like `GetNoteEmitter`? I mean do we really need to put `Visitor` in the name?


================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:111
 
+bool isStdSmartPtrCall(const CXXRecordDecl *Rec) {
+  if (!Rec || !Rec->getDeclContext()->isStdNamespace())
----------------
My guess is that it should be just `isStdSmartPtr`


================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:171
+
+PathDiagnosticPieceRef EmitNoteOnGetVisitor::emitBugReportAtGet(
+    const ExplodedNode *Node, BugReporterContext &BRC, const Expr *E) {
----------------
I'm still here picking on names 😅 It doesn't emit "bug report" it emits "note".
And actually if you name your class as `GetNoteEmitter` or something similar you don't need to mention `get` here again.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:198-202
+  const auto *E = llvm::dyn_cast<ImplicitCastExpr>(S);
+  if (!E || E->getCastKind() != CastKind::CK_PointerToBoolean)
+    return;
+  // Check if we are tracking the expression being casted
+  const Expr *Sub = E->getSubExpr();
----------------
I have two major questions about this implementation:

  - Why don't we need an actual check for `IfStmt`? Won't it trigger on `bool unused = !pointer;`? And if so it won't mean **constrained**.
  - Why do we only care about implicit pointer-to-bool conversion? What about situations like `pointer == nullptr`, `NULL != pointer`, `__builtin_expect(pointer, 0)`, etc?



================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:230
+  // If b is just a pointer, then we should add it to the set
+  if (const auto *ICE = llvm::dyn_cast<ImplicitCastExpr>(RHS)) {
+    const Expr *Sub = ICE->getSubExpr();
----------------
It's better to use `IgnoreParenCasts` here as well.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:79
       {{"get"}, &SmartPtrModeling::handleGet}};
+  mutable llvm::ImmutableSet<const Expr *>::Factory StmtSetFactory;
 };
----------------
vsavchenko wrote:
> I think it's better to use `REGISTER_SET_FACTORY_WITH_PROGRAMSTATE` instead and keep factory as part of the state as well.
It is marked as "Done", but the code is same.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D97183/new/

https://reviews.llvm.org/D97183