steakhal added a comment.
Add a test where we have multiple imp derived to base casts but only one leads
to a bug. Do it for both ways, once that the first is the offending, and once
the second.
================
Comment at: clang/docs/analyzer/checkers.rst:1793-1803
+.. code-block:: cpp
+
+ Base *create() {
+ Base *x = new Derived[10]; // note: conversion from derived to base
+ // happened here
+ return x;
+ }
----------------
Discookie wrote:
> steakhal wrote:
> > Make sure in the example the `create` is related (e.g. called/used).
> > Also, refrain from using `sink` in the docs. It's usually used in the
> > context of taint analysis.
> Changed the example - should I change the DeleteWithNonVirtualDtor example as
> well? That has the same issues as you said here.
I have no opinion. You could.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/CXXDeleteChecker.cpp:126
+
+ if (!DerivedClass->isDerivedFrom(BaseClass))
+ return;
----------------
Is this transitive?
BTW inheritance can only be expressed if the class is a definition, right?
Thus passing this should imply has definition.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/CXXDeleteChecker.cpp:143
+ R->markInteresting(BaseClassRegion);
+ R->addVisitor(std::make_unique<PtrCastVisitor>());
+ C.emitReport(std::move(R));
----------------
I think addVisitor already takes a template param, and it will call make unique
for you.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/CXXDeleteChecker.cpp:192
+
+ const Stmt *S = N->getStmtForDiagnostics();
+ if (!S)
----------------
Aren't you actually interested in N->getLocation().getAs<StmtPoint>().getStmt()?
The diag stmt can be fuzzy, but the PP is exact.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/CXXDeleteChecker.cpp:196
+
+ const auto *CastE = dyn_cast<CastExpr>(S);
+ if (!CastE)
----------------
If you dyncast to ImplicitCastExpr, couldn't you have done it here?
================
Comment at: clang/lib/StaticAnalyzer/Checkers/CXXDeleteChecker.cpp:202
+ // Explicit casts can have different CastKinds.
+ if (const auto *ImplCastE = dyn_cast<ImplicitCastExpr>(CastE)) {
+ if (ImplCastE->getCastKind() != CK_DerivedToBase)
----------------
How do you know that that this castexpr corresponds to the region for which you
report the bug? To mez this might be some unrelated castexpr.
I was expecting the offending memregion being passed to the visitor, that it
can check against.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/CXXDeleteChecker.cpp:217
+ // Stop traversal on this path.
+ Satisfied = true;
+
----------------
There are so many early returns going on. I feel, we could miss the program
point where it should have been marked satisfied. After this point, the visitor
will never or should never emit a note.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/CXXDeleteChecker.cpp:224
+ N->getLocationContext());
+ return std::make_shared<PathDiagnosticEventPiece>(Pos, OS.str(), true);
+}
----------------
Use named argument passing.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D158156/new/
https://reviews.llvm.org/D158156
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
