[PATCH] D94476: [analyzer] Implement conversion from Clang diagnostics to PathDiagnostics.

[Valeriy Savchenko via Phabricator via cfe-commits]

vsavchenko added inline comments.


================
Comment at: clang/include/clang/Analysis/PathDiagnosticConverterDiagnosticConsumer.h:23
+
+class PathDiagnosticConverterDiagnosticConsumer : public DiagnosticConsumer {
+public:
----------------
I think that this class (or file?) needs to have some comments on how it is supposed to be used.


================
Comment at: clang/lib/Analysis/PathDiagnosticConverterDiagnosticConsumer.cpp:38
+  Msg[0] = toupper(Msg[0]);
+  std::string CleanMsg = Msg.str().substr(0, Msg.str().find('[') - 1).str();
+
----------------
I think this type of stuff should be covered by a comment or a descriptive function name.


================
Comment at: clang/unittests/Analysis/PathDiagnosticConverterDiagnosticConsumer.cpp:1
+//===- unittests/Analysis/CFGTest.cpp - CFG tests -------------------------===//
+//
----------------
Looks like a copy-paste error :)


================
Comment at: clang/unittests/Analysis/PathDiagnosticConverterDiagnosticConsumer.cpp:63
+      size_t PieceI = 0;
+      for (const auto &Piece: Pieces) {
+        const ExpectedPieceTy &ExpectedPiece = ExpectedDiag.Pieces[PieceI];
----------------
nit


================
Comment at: clang/unittests/Analysis/PathDiagnosticConverterDiagnosticConsumer.cpp:63
+      size_t PieceI = 0;
+      for (const auto &Piece: Pieces) {
+        const ExpectedPieceTy &ExpectedPiece = ExpectedDiag.Pieces[PieceI];
----------------
vsavchenko wrote:
> nit
nit: `llvm::enumerate` or `llvm::zip`?


================
Comment at: clang/unittests/Analysis/PathDiagnosticConverterDiagnosticConsumer.cpp:83
+
+  void performTest(const Decl *D) {
+    TestPathDiagnosticConsumer::ExpectedDiagsTy ExpectedDiags{
----------------
TBH it is pretty hard to follow the test and from it's structure see what is the input and what is expected output.  Maybe it can be reorganized a bit?


================
Comment at: clang/unittests/Analysis/PathDiagnosticConverterDiagnosticConsumer.cpp:139
+
+class PathDiagnosticConverterDiagnosticConsumerTestAction
+    : public ASTFrontendAction {
----------------
WE NEED MORE NOUNS!


================
Comment at: clang/unittests/Analysis/PathDiagnosticConverterDiagnosticConsumer.cpp:149-150
+
+// Constructing a CFG for a range-based for over a dependent type fails (but
+// should not crash).
+TEST(PathDiagnosticConverter, ConsumeWarning) {
----------------
Also doesn't seem relevant


================
Comment at: clang/unittests/Analysis/PathDiagnosticConverterDiagnosticConsumer.cpp:154
+      std::make_unique<PathDiagnosticConverterDiagnosticConsumerTestAction>(),
+      "void foo() {}", "input.c"));
+}
----------------
It is also about the structure, I guess it would've been nice to have all the inputs and expected outputs to be specified here in the actual test, so the reader can figure out what is tested without diving deep into the classes. And it also seems that with the current structure you'll need a couple more classes for every new test.


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

https://reviews.llvm.org/D94476