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

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added inline comments.


================
Comment at: clang/lib/Analysis/PathDiagnosticConverterDiagnosticConsumer.cpp:1
+//===--- PathDiagnosticConverterDiagnosticConsumer.cpp ----------*- C++ -*-===//
+//
----------------
steakhal wrote:
> I've seen this a few times, and I still don't know why we use it.
> The file extension should be enough to recognize that this is a C++ file.
> Can someone clarify this?
https://llvm.org/docs/CodingStandards.html#file-headers

(with our file name it doesn't look like there's much space even for a short description)
(i should probably convert the long description into a doxygen comment as well)


================
Comment at: clang/lib/Analysis/PathDiagnosticConverterDiagnosticConsumer.cpp:14
+
+#include <clang/Analysis/PathDiagnosticConverterDiagnosticConsumer.h>
+
----------------
steakhal wrote:
> I frequently see `#include "clang/..."`-ish includes but never `#include <clang/...>`.
Whoops thanks.


================
Comment at: clang/lib/Analysis/PathDiagnosticConverterDiagnosticConsumer.cpp:20
+void PathDiagnosticConverterDiagnosticConsumer::flushPartialDiagnostic() {
+  if (PartialPDs.empty())
+    return;
----------------
xazax.hun wrote:
> Do we need this early return? We might get the same behavior by simply omitting this check. I have no strong preference about keeping or removing it.
We need it. @steakhal figured this out correctly in the next comment.


================
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();
+
----------------
steakhal wrote:
> vsavchenko wrote:
> > I think this type of stuff should be covered by a comment or a descriptive function name.
> Eh, I don't think it's gonna work this way.
> We can not assume that the `[` won't appear in the payload of the message.
> Eg.:
> `NewDelete-checker-test.cpp:193`
> ```
> // newdelete-warning{{Argument to 'delete[]' is the address of the local variable 'i', which is not memory allocated by 'new[]'}}
> ```
> 
> The best you could do is to do a reverse search.
> Do we emit the `[mypackage.mychecker]` suffix for all the reports? If not, then we have a problem.
Uh-oh, mmm, indeed. I should definitely make this optional as well.


================
Comment at: clang/lib/Analysis/PathDiagnosticConverterDiagnosticConsumer.cpp:54-60
+      PathDiagnosticPieceRef Piece;
+      if (ShouldDisplayNotesAsEvents) {
+        Piece = std::make_shared<PathDiagnosticEventPiece>(PDLoc, Msg);
+      } else {
+        Piece = std::make_shared<PathDiagnosticNotePiece>(PDLoc, Msg);
+      }
+      PartialPDs[Consumer]->getMutablePieces().push_back(Piece);
----------------
steakhal wrote:
> The ternary operator could simplify this inside the `push_back` function - `emplace_back`?
Can't easily use `?:` here because LHS and RHS are of different types and operator `?:` doesn't do implicit casts.


================
Comment at: clang/lib/Analysis/PathDiagnosticConverterDiagnosticConsumer.cpp:71
+          BTI.CheckName, /*DeclWithIssue=*/nullptr, BTI.BugType,
+          CleanMsg, CleanMsg, BTI.BugCategory, PDLoc, /*DeclToUnique=*/nullptr,
+          std::make_unique<FilesToLineNumsMap>());
----------------
steakhal wrote:
> Should Desc and ShortDesc be the same?
They don't need to be the same but typically clang diagnostics don't provide two different wordings for the same warning.


================
Comment at: clang/unittests/Analysis/PathDiagnosticConverterDiagnosticConsumer.cpp:44
+
+  virtual StringRef getName() const override { return "test"; }
+
----------------
steakhal wrote:
> I would suggest removing the redundant `virtual`. The same applies to the other decls.
Yup, should've said `override` instead.


================
Comment at: clang/unittests/Analysis/PathDiagnosticConverterDiagnosticConsumer.cpp:154
+      std::make_unique<PathDiagnosticConverterDiagnosticConsumerTestAction>(),
+      "void foo() {}", "input.c"));
+}
----------------
vsavchenko wrote:
> 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.
We have to follow the `libTooling` architecture where we have to have a `FrontendAction` class and an `ASTConsumer` class with specific callback signatures. I'll try to think of something.


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

https://reviews.llvm.org/D94476