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