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

Mon Nov 25 14:21:03 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:
> > > 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