[PATCH] D69782: Summary: Instead of dropping all the ranges associated with a Diagnostic when converting them to a ClangTidy error, instead attach them to the ClangTidyError, so they can be consumed by other APIs.

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh added a comment.

Thanks for the update!

Please upload patches with full context. That makes navigating the code much easier during reviews. See https://llvm.org/docs/Phabricator.html

A few more comments inline.



================
Comment at: include/clang/Tooling/Core/Diagnostic.h:62-70
+/// Represents extra source ranges to be associated with a diagnostic.
+struct DiagnosticAssociatedRanges {
+  DiagnosticAssociatedRanges() = default;
+
+  DiagnosticAssociatedRanges(const SourceManager &Sources,
+                             ArrayRef<CharSourceRange> SourceRanges);
+
----------------
Why is this needed? Shouldn't `LLVM_YAML_IS_SEQUENCE_VECTOR` be enough to allow for SmallVector<DiagnosticAssociatedRange, ...> to be yaml serializable? Seems to work with `DiagnosticMessage` and `Diagnostic::Notes`.


================
Comment at: lib/Tooling/Core/Diagnostic.cpp:51
+  for (const CharSourceRange &Range : SourceRanges) {
+    Ranges.emplace_back(DiagnosticAssociatedRange(Sources, Range));
+  }
----------------
With emplace_back constructor parameters can be used directly:

  Ranges.emplace_back(Sources, Range);

That's the whole point of `emplace_back`.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69782/new/

https://reviews.llvm.org/D69782