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