[PATCH] D76605: [analyzer] Display the checker name in the text output

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

Szelethus marked 3 inline comments as done.
Szelethus added inline comments.


================
Comment at: clang/include/clang/StaticAnalyzer/Core/AnalyzerOptions.def:313-316
+ANALYZER_OPTION(bool, ShouldDisplayCheckerNameForText, "display-checker-name",
+                "Display the checker name for textual outputs",
+                true)
+
----------------
NoQ wrote:
> Why do we need an option? Is it just for tests? Is it for clang-tidy to avoid printing the checker name twice?
> 
> Why do we need an option?

Well, we don't, but there is just no reason not to make this configurable, as seen in the debug checker test file. It would be unnecessary noise in their case.

> Is it just for tests?

Tests are a big motivating factor, but it just doesn't hurt to know (just like in clang-tidy!). I suspect this change affects developers or powerusers more than any others, and could accelerate debugging and/or configuring just a tiny bit more.

> Is it for clang-tidy to avoid printing the checker name twice?

Clang-tidy isnt affected, as they use the PD_NONE output type, not PD_TEXT.


================
Comment at: clang/lib/StaticAnalyzer/Core/TextDiagnostics.cpp:112
         reportPiece(NoteID, Piece->getLocation().asLocation(),
-                    Piece->getString(), Piece->getRanges(), Piece->getFixits());
+                    Piece->getString().str(), Piece->getRanges(),
+                    Piece->getFixits());
----------------
NoQ wrote:
> Szelethus wrote:
> > martong wrote:
> > > Why the `.str()` ?
> > `StringRef` no longer converts to `std::string` implicitly.
> But it seems to have been fine before(?)
Mind that I changed the parameter of `reportPiece` to `std::string` from `StringRef`.


================
Comment at: clang/test/Analysis/incorrect-checker-names.cpp:6
+  int x = 0;
+  // FIXME: This shouldn't be tied to a modeling checker.
+  return &x; // expected-warning{{Address of stack memory associated with local variable 'x' returned to caller [core.StackAddrEscapeBase]}}
----------------
NoQ wrote:
> Do you have a plan to address this FIXME? Does clang-tidy have the same problem?
It does not. This is the issue of emitting reports under the wrong name, and I do have plans to address it, though I'm very anxious about `RetainCountChecker`. Its huge, the modeling/bug emission parts of it a very much intertwined, and me dum dum about ObjC.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76605