[PATCH] D26137: [clang-tidy] Add check name to YAML export

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh requested changes to this revision.
alexfh added a comment.
This revision now requires changes to proceed.

Looks mostly good. A few more nits.



================
Comment at: include/clang/Tooling/Core/Diagnostic.h:68-71
+  /// A freeform chunk of text to describe the context of the replacements.
+  /// Will be printed, for example, when detecting conflicts during replacement
+  /// deduplication.
+  std::string Context;
----------------
Alpha wrote:
> alexfh wrote:
> > That's too vague. Are you intending to use it only for reporting problems with replacement deduplication? Should it be in this structure at all?
> This was actually left to keep compatibility with `TranslationUnitReplacements` which was used for the export.
> But it seems that even for that structure, there is in all likelihood no reference to any use of the `Context` field, except in test cases and in the Yaml IO mapping, where it is marked as an optional entry.
> Should it be discarded instead (here, and thus also in `TranslationUnitReplacements`)?
Yes, it seems we can just remove the `Context` field.


================
Comment at: include/clang/Tooling/Core/Diagnostic.h:36
+  /// \brief Constructs a diagnostic message with anoffset to the diagnostic
+  /// within the file where the problem occured
+  ///
----------------
nit: Add a trailing period.


================
Comment at: include/clang/Tooling/DiagnosticsYaml.h:35
+    NormalizedDiagnostic(const IO &)
+        : DiagnosticName(""), Message(), Fix(), Notes(), DiagLevel() {}
+
----------------
Initializers for non-trivial members are superfluous. Looks like only `DiagLevel` initializer makes sense, but I'd use an explicit value for it. 


================
Comment at: include/clang/Tooling/DiagnosticsYaml.h:58
+    std::vector<clang::tooling::Replacement> Fixes;
+    for (auto & Replacements : Keys->Fix) {
+       for (auto & Replacement : Replacements.second) {
----------------
clang-format, please


================
Comment at: include/clang/Tooling/DiagnosticsYaml.h:79
+      for (auto &Diagnostic : Doc.Diagnostics) {
+        if (Diagnostic.Fix.size() > 0) {
+          Diagnostics.push_back(Diagnostic);
----------------
Should we copy all diagnostics instead?


================
Comment at: tools/extra/clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp:42
+                                 TUDiagnostics &TUs,
+                                 TUDiagnosticFiles & TURFiles,
                                  clang::DiagnosticsEngine &Diagnostics) {
----------------
clang-format, please


================
Comment at: tools/extra/clang-tidy/ClangTidyDiagnosticConsumer.h:56
   // Note: it is empty in unittest.
   std::string BuildDirectory;
 
----------------
It might make sense to move BuildDirectory to clang::Tooling::Diagnostic as well.


Repository:
  rL LLVM

https://reviews.llvm.org/D26137