vsavchenko added inline comments.
================
Comment at:
clang/include/clang/Analysis/PathDiagnosticConverterDiagnosticConsumer.h:23
+
+class PathDiagnosticConverterDiagnosticConsumer : public DiagnosticConsumer {
+public:
----------------
I think that this class (or file?) needs to have some comments on how it is
supposed to be used.
================
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();
+
----------------
I think this type of stuff should be covered by a comment or a descriptive
function name.
================
Comment at:
clang/unittests/Analysis/PathDiagnosticConverterDiagnosticConsumer.cpp:1
+//===- unittests/Analysis/CFGTest.cpp - CFG tests
-------------------------===//
+//
----------------
Looks like a copy-paste error :)
================
Comment at:
clang/unittests/Analysis/PathDiagnosticConverterDiagnosticConsumer.cpp:63
+ size_t PieceI = 0;
+ for (const auto &Piece: Pieces) {
+ const ExpectedPieceTy &ExpectedPiece = ExpectedDiag.Pieces[PieceI];
----------------
nit
================
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:
> nit
nit: `llvm::enumerate` or `llvm::zip`?
================
Comment at:
clang/unittests/Analysis/PathDiagnosticConverterDiagnosticConsumer.cpp:83
+
+ void performTest(const Decl *D) {
+ TestPathDiagnosticConsumer::ExpectedDiagsTy ExpectedDiags{
----------------
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?
================
Comment at:
clang/unittests/Analysis/PathDiagnosticConverterDiagnosticConsumer.cpp:139
+
+class PathDiagnosticConverterDiagnosticConsumerTestAction
+ : public ASTFrontendAction {
----------------
WE NEED MORE NOUNS!
================
Comment at:
clang/unittests/Analysis/PathDiagnosticConverterDiagnosticConsumer.cpp:149-150
+
+// Constructing a CFG for a range-based for over a dependent type fails (but
+// should not crash).
+TEST(PathDiagnosticConverter, ConsumeWarning) {
----------------
Also doesn't seem relevant
================
Comment at:
clang/unittests/Analysis/PathDiagnosticConverterDiagnosticConsumer.cpp:154
+ std::make_unique<PathDiagnosticConverterDiagnosticConsumerTestAction>(),
+ "void foo() {}", "input.c"));
+}
----------------
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.
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
