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

[Balázs Benics via Phabricator via cfe-commits]

steakhal added a comment.

Seems pretty straightforward and clean.

The cleanup of the report's message should be reworked.
Besides, that looks good to me.

I think these cases should be tested as well:

- [Warning, Warning, Warning]
- [Warning, Note, Note]
- [Warning, Note, Note, Warning, Note]
- [Warning, Note, Remark, Warning, Remark]

PS: clang-format; sorry for the nit-spam :D



================
Comment at: clang/include/clang/Analysis/PathDiagnosticConverterDiagnosticConsumer.h:53-56
+  /// Inform the consumer that the last diagnostic has been sent. This is
+  /// necessary because the consumer does not know whether more notes are
+  /// going to be attached to the last warning.
+  void finalize() { flushPartialDiagnostic(); }
----------------
Overriding the `virtual void clang::DiagnosticConsumer::finish()` can't accomplish the same thing?


================
Comment at: clang/lib/Analysis/PathDiagnosticConverterDiagnosticConsumer.cpp:1
+//===--- PathDiagnosticConverterDiagnosticConsumer.cpp ----------*- C++ -*-===//
+//
----------------
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?


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


================
Comment at: clang/lib/Analysis/PathDiagnosticConverterDiagnosticConsumer.cpp:23
+
+  for (PathDiagnosticConsumer *Consumer : PathConsumers) {
+    Consumer->HandlePathDiagnostic(std::move(PartialPDs[Consumer]));
----------------



================
Comment at: clang/lib/Analysis/PathDiagnosticConverterDiagnosticConsumer.cpp:37
+  Info.FormatDiagnostic(Msg);
+  Msg[0] = toupper(Msg[0]);
+  std::string CleanMsg = Msg.str().substr(0, Msg.str().find('[') - 1).str();
----------------



================
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();
+
----------------
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.


================
Comment at: clang/lib/Analysis/PathDiagnosticConverterDiagnosticConsumer.cpp:45-52
+  // For now we assume that every diagnostic is either a warning (or something
+  // similar, like an error) or a note logically attached to a previous warning.
+  // Notes do not come in actually attached to their respective warnings
+  // but are fed to us independently. The good news is they always follow their
+  // respective warnings and come in in the right order so we can
+  // automatically re-attach them.
+  // FIXME: Handle other stuff, like remarks(?)
----------------
So, `Error` and `Fatal`is treated in the same way as `Warning`.
What about the `Ignored`?


================
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);
----------------
The ternary operator could simplify this inside the `push_back` function - `emplace_back`?


================
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>());
----------------
Should Desc and ShortDesc be the same?


================
Comment at: clang/lib/Analysis/PathDiagnosticConverterDiagnosticConsumer.cpp:74-77
+      PathDiagnosticPieceRef Piece =
+          std::make_shared<PathDiagnosticEventPiece>(PDLoc, CleanMsg);
+
+      PD->setEndOfPath(std::move(Piece));
----------------
Why not construct in place?


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


================
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:
> vsavchenko wrote:
> > nit
> nit: `llvm::enumerate` or `llvm::zip`?
`clang-format` should format these files as well, am I right?


================
Comment at: clang/unittests/Analysis/PathDiagnosticConverterDiagnosticConsumer.cpp:83
+
+  void performTest(const Decl *D) {
+    TestPathDiagnosticConsumer::ExpectedDiagsTy ExpectedDiags{
----------------
vsavchenko wrote:
> 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?
I can confirm.
What about passing the `ExpectedDiags` as a parameter?
Probably some shorter type alias could come handy for `TestPathDiagnosticConsumer::ExpectedDiagTy` and `TestPathDiagnosticConsumer::ExpectedPieceTy`.


================
Comment at: clang/unittests/Analysis/PathDiagnosticConverterDiagnosticConsumer.cpp:139
+
+class PathDiagnosticConverterDiagnosticConsumerTestAction
+    : public ASTFrontendAction {
----------------
vsavchenko wrote:
> WE NEED MORE NOUNS!
What about calling this `DiagnosticMock`?


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

https://reviews.llvm.org/D94476