rnk marked an inline comment as done. rnk added inline comments.
================ Comment at: clang/include/clang/AST/Attrs.h:1 +//===--- Attr.h - Classes for representing attributes ----------*- C++ -*-===// +// ---------------- aaron.ballman wrote: > rnk wrote: > > aaron.ballman wrote: > > > rnk wrote: > > > > aaron.ballman wrote: > > > > > This should read `Attrs.h` instead, but I find the naming distinction > > > > > to be a bit too subtle. How about `AttrSubclasses.h` or > > > > > `ConcreteAttrs.h` (other suggestions welcome too). > > > > WDYT about following Decl.h / DeclBase.h? So, rename Attr.h to > > > > AttrBase.h, update all current includers of Attr.h to AttrBase.h, then > > > > add in new includes of Attr.h. Perhaps staged as two commits, one to do > > > > the rename, one to introduce the new header. > > > > > > > > Otherwise, I guess I like AttrSubclasses.h from your list, or > > > > AttrClasses.h as an alternative. > > > I think AttrBase.h for the `Attr` base class makes a lot of sense -- it > > > makes it more clear that you only get the base class. WDYT about > > > `Attrs.h` for the subclasses instead of `Attr.h` -- might make it a bit > > > more clear that you're getting all of the attributes, not just the base > > > class? > > That makes sense. Do you mind if I land this first, and then do the Attr.h > > -> AttrBase.h rename? That way I don't have to make a new patch, put it up > > for review, and rebase this patch over it. I'll send a review assuming the > > answer is yes and then reorder them if you have concerns. > Totally fine with that approach, thanks for checking! Hm, I just realized that this will create a lot of churn for out of tree includers of Attr.h. Chrome's plugins have three hits: https://cs.chromium.org/search/?q=include.*AST/Attr%5C.h&sq=package:chromium&type=cs Other users will have more. Clang doesn't promise API stability, so we can certainly do this, but it creates a fair volume of work. Now I'm considering splitting out AttrBase.h on its own and updating as many includes of Attr.h to use that as possible. That'll end up being a much less disruptive and much smaller change. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D70627/new/ https://reviews.llvm.org/D70627 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
