[PATCH] D26137: [clang-tidy] Add check name to YAML export
Alexander Kornienko via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Tue Jan 3 05:48:46 PST 2017
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
More information about the cfe-commits
mailing list