aaron.ballman added inline comments.
================ Comment at: clang/include/clang/AST/Attrs.h:1 +//===--- Attr.h - Classes for representing attributes ----------*- C++ -*-===// +// ---------------- 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! 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
