[PATCH] D34404: [Clang-Tidy] Preserve Message, FileOffset, FilePath in Clang-Tidy YAML output

[Alexander Kornienko via Phabricator via cfe-commits]

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