[PATCH] D58777: [analyzer] Fix an assertation failurure for invalid sourcelocation, add a new debug checker

[Kristóf Umann via Phabricator via cfe-commits]

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