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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
