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

Alpha Abdoulaye via cfe-commits cfe-commits at lists.llvm.org
Tue Nov 8 08:07:55 PST 2016


Alpha added inline comments.


================
Comment at: include/clang/Tooling/Core/Diagnostic.h:35
+  DiagnosticMessage(llvm::StringRef Message, const SourceManager &Sources,
+                    SourceLocation Loc);
+  std::string Message;
----------------
alexfh wrote:
> What are the constraints on the location? Should it be a file location or macro locations are fine too? Please add a (doxygen-style) comment.
Indeed, this should be a file location.


================
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;
----------------
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`)?


================
Comment at: tools/extra/clang-tidy/ClangTidy.cpp:578
                         raw_ostream &OS) {
-  TranslationUnitReplacements TUR;
-  for (const ClangTidyError &Error : Errors) {
-    for (const auto &FileAndFixes : Error.Fix)
-      TUR.Replacements.insert(TUR.Replacements.end(),
-                              FileAndFixes.second.begin(),
-                              FileAndFixes.second.end());
-  }
-
+  TranslationUnitDiagnostics TUD;
+  TUD.Diagnostics.insert(TUD.Diagnostics.end(), Errors.begin(), Errors.end());
----------------
alexfh wrote:
> This function neither fills `TUD.MainSourceFile` nor `TUD.Context` (which I'm not sure even belongs to this structure).
Done for  `MainSourceFile` which was surprisingly never exported with the fixes.
For  `Context`, see above comment about the 'TranslationUnitDiagnostics' structure.


Repository:
  rL LLVM

https://reviews.llvm.org/D26137





More information about the cfe-commits mailing list