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