[PATCH] D70627: [AST] Split out Attrs.h for table generated *Attr classes

Mon Nov 25 14:10:42 PST 2019

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:
> > > 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.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70627