[PATCH] D38772: [refactor] allow the use of refactoring diagnostics

[Alex Lorenz via Phabricator via cfe-commits]

arphaman added inline comments.


================
Comment at: lib/Basic/DiagnosticIDs.cpp:46
   unsigned WarnShowInSystemHeader : 1;
-  unsigned Category : 5;
+  unsigned Category : 6;
 
----------------
hokein wrote:
> just curious: is this change needed?
I get a build warning without this change as the bitfield becomes too narrow with the new category, so yeah.


================
Comment at: tools/clang-refactor/ToolRefactoringResultConsumer.h:19
+
+/// A subclass of \c RefactoringResultConsumer that stores the reference to the
+/// TU-specific diagnostics engine.
----------------
hokein wrote:
> I'd name it "interface", because it has unimplemented virtual function (`handleError`), clients can't create an instance of it. 
> 
> or alternatively,  does it make more sense to just add these methods and `DiagnosticsEngine` variable to the `tooling::RefactoringResultConsumer` interface? I see you have replaced "RefactoringResultConsumer" with this new interface in many places. 
Right now I don't think having the diagnostics engine will be useful for clients outside of tool, so I'd prefer to keep it here. We can reconsider this decision in the future if we need to.


Repository:
  rL LLVM

https://reviews.llvm.org/D38772