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

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh added a comment.

Err, looks like I forgot to post comments I entered a few days ago.

Just a few nits.



================
Comment at: include/clang/Tooling/Core/Diagnostic.h:62
+  Diagnostic(llvm::StringRef DiagnosticName, DiagnosticMessage &Message,
+             llvm::StringMap<tooling::Replacements> &Fix,
+             SmallVector<DiagnosticMessage, 1> &Notes, Level DiagLevel,
----------------
nit: No need for `tooling::`.


================
Comment at: lib/Tooling/Core/Diagnostic.cpp:39
+                       DiagnosticMessage &Message,
+                       llvm::StringMap<tooling::Replacements> &Fix,
+                       SmallVector<DiagnosticMessage, 1> &Notes,
----------------
nit: No need for `tooling::`.


================
Comment at: tools/extra/clang-tidy/ClangTidy.cpp:106
   void reportDiagnostic(const ClangTidyError &Error) {
-    const ClangTidyMessage &Message = Error.Message;
+    const ClangTidyMessage Message = Error.Message;
     SourceLocation Loc = getLocation(Message.FilePath, Message.FileOffset);
----------------
Why this change?


================
Comment at: tools/extra/clang-tidy/ClangTidy.h:245
 /// output stream.
-void exportReplacements(const std::vector<ClangTidyError> &Errors,
+void exportReplacements(const StringRef MainFilePath,
+                        const std::vector<ClangTidyError> &Errors,
----------------
Top-level const on function parameters in function declarations is useless (it's not a part of the function prototype and it tells nothing to the function users). It might make sense on a function definition, if used consistently (same way as on local variables). However, is not very common in LLVM/Clang.


================
Comment at: tools/extra/clang-tidy/ClangTidyDiagnosticConsumer.h:45
 /// FIXME: Make Diagnostics flexible enough to support this directly.
-struct ClangTidyError {
-  enum Level {
-    Warning = DiagnosticsEngine::Warning,
-    Error = DiagnosticsEngine::Error
-  };
-
-  ClangTidyError(StringRef CheckName, Level DiagLevel, bool IsWarningAsError,
-                 StringRef BuildDirectory);
-
-  std::string CheckName;
-  ClangTidyMessage Message;
-  // Fixes grouped by file path.
-  llvm::StringMap<tooling::Replacements> Fix;
-  SmallVector<ClangTidyMessage, 1> Notes;
-
-  // A build directory of the diagnostic source file.
-  //
-  // It's an absolute path which is `directory` field of the source file in
-  // compilation database. If users don't specify the compilation database
-  // directory, it is the current directory where clang-tidy runs.
-  //
-  // Note: it is empty in unittest.
-  std::string BuildDirectory;
+struct ClangTidyError : clang::tooling::Diagnostic {
+  ClangTidyError(StringRef CheckName, Level DiagLevel, StringRef BuildDirectory,
----------------
nit: No need for `clang::`.


================
Comment at: tools/extra/clang-tidy/ClangTidyDiagnosticConsumer.h:36
 
-/// \brief A message from a clang-tidy check.
-///
-/// Note that this is independent of a \c SourceManager.
-struct ClangTidyMessage {
-  ClangTidyMessage(StringRef Message = "");
-  ClangTidyMessage(StringRef Message, const SourceManager &Sources,
-                   SourceLocation Loc);
-  std::string Message;
-  std::string FilePath;
-  unsigned FileOffset;
-};
+typedef clang::tooling::DiagnosticMessage ClangTidyMessage;
 
----------------
Do we actually need this typedef? How much is the old type name used?


Repository:
  rL LLVM

https://reviews.llvm.org/D26137