alexfh requested changes to this revision.
alexfh added inline comments.
This revision now requires changes to proceed.
================
Comment at: unittests/Tooling/DiagnosticsYamlTest.cpp:51
+
+ ASSERT_STREQ("---\n"
+ "MainSourceFile: path/to/source.cpp\n"
----------------
EXPECT_STREQ is more appropriate here. ASSERT_* family of macros terminates the
test, and thus should only be used to verify the invariants that are absolutely
required to continue the execution of the test, which is not the case here.
================
Comment at: unittests/clang-apply-replacements/ApplyReplacementsTest.cpp:18
+
+static tooling::Diagnostic makeDiagnostic(const StringRef DiagnosticName,
+ DiagnosticMessage &Message,
----------------
I'd remove `const` from `DiagnosticName`, since a StringRef doesn't allow to
modify the string it points to (and marking by value arguments `const` is not
common in LLVM). However, `Message` and `Replacements` allow to modify the
referenced objects and thus should be marked `const`.
Also consider inlining this method, it doesn't buy you much.
================
Comment at: unittests/clang-apply-replacements/ApplyReplacementsTest.cpp:23
+ SmallVector<DiagnosticMessage, 1> EmptyNotes;
+ return tooling::Diagnostic(DiagnosticName, Message, Replacements, EmptyNotes,
+ tooling::Diagnostic::Warning, BuildDirectory);
----------------
Will `{}` work instead of `EmptyNotes`?
================
Comment at: unittests/clang-apply-replacements/ApplyReplacementsTest.cpp:23
+ SmallVector<DiagnosticMessage, 1> EmptyNotes;
+ return tooling::Diagnostic(DiagnosticName, Message, Replacements, EmptyNotes,
+ tooling::Diagnostic::Warning, BuildDirectory);
----------------
alexfh wrote:
> Will `{}` work instead of `EmptyNotes`?
`tooling::` is not needed due to the using directive above. Same below.
================
Comment at: unittests/clang-apply-replacements/ApplyReplacementsTest.cpp:54-56
+ bool success = mergeAndDeduplicate(TUs, ReplacementsMap, SM);
+
+ EXPECT_TRUE(success);
----------------
I'd remove the `success` variable and just call EXPECT_TRUE(merge...)
Repository:
rL LLVM
https://reviews.llvm.org/D34404
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
