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