Szelethus added inline comments.
================
Comment at: lib/StaticAnalyzer/Checkers/DebugCheckers.cpp:271
+//===----------------------------------------------------------------------===//
+// Emits a report for each and every Stmt.
+//===----------------------------------------------------------------------===//
----------------
NoQ wrote:
> 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?
>Also would you eventually want to expand this checker with non-Stmt callbacks?
No :)
================
Comment at: lib/StaticAnalyzer/Core/PathDiagnostic.cpp:679
+
+ if (ME->getMemberLoc().isValid())
+ return PathDiagnosticLocation(ME->getMemberLoc(), SM, SingleLocK);
----------------
NoQ wrote:
> I think it's worth commenting why exactly this may fail. It's fairly hard to
> guess that we're talking about member operators.
I mean, I have absolutely no idea how this happened :') The best I can do is to
document that it does happen from time to time and why this is the solution to
that.
================
Comment at: test/Analysis/invalid-pos-fix.cpp:6
+
+struct h {
+ operator int();
----------------
NoQ wrote:
> 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 :]
>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 :]
I always wondered why we are all bunched up in `test/Analysis`.
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
