[PATCH] D68795: [libTooling] Introduce a single, "main" header file for Transformer.

Thu Oct 10 10:53:59 PDT 2019

ymandel marked an inline comment as done.
ymandel added inline comments.

================
Comment at: clang/include/clang/Tooling/Transformer/Transformer.h:28
+// also update their code to reference `TransformerTool`.
+#include "clang/Tooling/Transformer/TransformerTool.h"
+// Temporary alias to assist clients in migrating to new class name.
----------------
ymandel wrote:
> gribozavr wrote:
> > I'm not a fan of umbrella headers... especially if they don't cover everything.
> > 
> > I think "include what you use" is a better rule.
> > 
> > The rest of the patch makes sense to me, just not this file. (OK to keep it temporarily if we need it to help clients migrate, of course.)
> Thanks, sounds good.  In that case, might it make more sense to just split off RewriteRule and not rename Transformer -> TransformerTool (along w/ the correpsonding header change)?  My primary motivation for renaming the class was to allow Transformer to be the name for the umbrella.
> 
> Since Transformer.h will have to include RewriteRule.h anyhow, there won't be any need for a forwarding header while we update users.
I've uploaded a new diff w/ these changes. I also split the implementation file, which I'd forgotten to do previously.  I've left the test in one file because testing the RewriteRule code w/o Transformer is somewhat complicated. It's probably worth doing that work, but doesn't seem critical at this point just for the sake of somewhat more independent unit tests.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68795