NoQ accepted this revision.
NoQ added a subscriber: george.karpenkov.
NoQ added a comment.
This revision is now accepted and ready to land.
Very nice! Crashing diagnostic locations are a fairly common issue. That said,
this still makes me wonder what sort of checker were you writing that triggered
this originally :)
================
Comment at: lib/StaticAnalyzer/Checkers/DebugCheckers.cpp:271
+//===----------------------------------------------------------------------===//
+// Emits a report for each and every Stmt.
+//===----------------------------------------------------------------------===//
----------------
Technically, we don't invoke `checkPreStmt` for every `Stmt` (eg., `IfStmt` is
omitted in favor of `checkBranchCondition`, `IntegerLiteral` is outright
skipped because `ExprEngine` doesn't even evaluate anything at this point).
Moreover, for some `Stmt`s we more-or-less-intentionally only invoke
`checkPreStmt` but not `checkPostStmt`.
Also would you eventually want to expand this checker with non-`Stmt` callbacks?
================
Comment at: lib/StaticAnalyzer/Checkers/DebugCheckers.cpp:277
+class ReportStmts : public Checker<check::PreStmt<Stmt>> {
+ std::unique_ptr<BuiltinBug> BT_stmtLoc;
+
----------------
My current favorite approach is:
```lang=c++
BuiltinBug BT_stmtLoc{this, "Statement"};
```
...and remove the constructor. And remove the `*` in your
`make_unique<BugReport>`.
================
Comment at: lib/StaticAnalyzer/Core/PathDiagnostic.cpp:574-575
+ // FIXME: Ironically, this assert actually fails in some cases.
+ //assert(L.isValid());
return L;
----------------
A normal day in Clang :)
================
Comment at: lib/StaticAnalyzer/Core/PathDiagnostic.cpp:679
+
+ if (ME->getMemberLoc().isValid())
+ return PathDiagnosticLocation(ME->getMemberLoc(), SM, SingleLocK);
----------------
I think it's worth commenting why exactly this may fail. It's fairly hard to
guess that we're talking about member operators.
================
Comment at: test/Analysis/invalid-pos-fix.cpp:6
+
+struct h {
+ operator int();
----------------
Random ideas on organizing tests so that to avoid adding thousands of one-test
files:
* Give these objects fancier names and/or add a `no-crash` so that it was clear
that this test tests a crash for reporting a bug over a `MemberExpr` that
corresponds to an `operator()`
* Or maybe wrap this into a namespace, so that it was easier to expand this
file.
* We traditionally put such tests into `test/Analysis/diagnostics`, dunno why
though.
etc.
I also recall @george.karpenkov arguing for making test directory structure
mirror source directory structure, but that's definitely not a blocker for this
patch :]
================
Comment at: test/Analysis/invalid-pos-fix.cpp:11-13
+ return h(); // expected-warning{{Statement}}
+ // expected-warning@-1{{Statement}}
+ // expected-warning@-2{{Statement}}
----------------
`// expected-warning 3 {{Statement}}`
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D58777/new/
https://reviews.llvm.org/D58777
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
