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

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

> [Warning, Note, Remark, Warning, Remark]

I excluded remarks for now with an assertion. Other tests added :)



================
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(); }
----------------
steakhal wrote:
> Overriding the `virtual void clang::DiagnosticConsumer::finish()` can't accomplish the same thing?
*mind blown*


================
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();
+
----------------
NoQ wrote:
> 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.
> Do we emit the [mypackage.mychecker] suffix for all the reports? If not, then we have a problem.

This was supposed to be a workaround for clang-tidy doing this to us. I think it's actually better to teach clang-tidy not to do this when it's pumping output to us. PathDiagnosticConsumers will re-add the checker name when appropriate (eg., in text output but not in plist output).


================
Comment at: clang/lib/Analysis/PathDiagnosticConverterDiagnosticConsumer.cpp:74-77
+      PathDiagnosticPieceRef Piece =
+          std::make_shared<PathDiagnosticEventPiece>(PDLoc, CleanMsg);
+
+      PD->setEndOfPath(std::move(Piece));
----------------
steakhal wrote:
> Why not construct in place?
With these classes typically there's a bunch of stuff in between, such as `Piece->addRange()`. Wait a sec, I can actually handle ranges pretty quickly. Let me add them so that this place wasn't empty :D


================
Comment at: clang/unittests/Analysis/PathDiagnosticConverterDiagnosticConsumer.cpp:139
+
+class PathDiagnosticConverterDiagnosticConsumerTestAction
+    : public ASTFrontendAction {
----------------
steakhal wrote:
> vsavchenko wrote:
> > WE NEED MORE NOUNS!
> What about calling this `DiagnosticMock`?
We still need to discriminate between `FrontendAction` mock, `ASTConsumer` mock, and `PathDiagnosticConsumer` mock. Dropped some of the nouns though.


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

https://reviews.llvm.org/D94476