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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits