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

[Reid Kleckner via Phabricator via cfe-commits]

rnk abandoned this revision.
rnk added a comment.

I think this isn't necessary. I think I got most of the value of this in 
rG60573ae6fe509b618dc6a2c5c55d921bccd77608 
, and we 
don't need two AttrBase.h / Attr.h headers in the long run. This is how many 
files depend on Attr.h now:

  $ ninja -t deps | grep 'AST.Attr\.h' | wc -l
  683

I think most deps are through Sema.h and ASTMatchers.h:

  $ ninja -t deps | grep 'Sema.Sema\.h' | wc -l
  102
  $ ninja -t deps | grep 'ASTMatchers.ASTMatchers\.h' | wc -l
  391


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

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

[Reid Kleckner via Phabricator via cfe-commits]

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

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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.

LGTM with the follow-up plan.


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

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

[Aaron Ballman via Phabricator via cfe-commits]

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

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

[Reid Kleckner via Phabricator via cfe-commits]

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



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Aaron Ballman via Phabricator via cfe-commits]

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


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

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

[Reid Kleckner via Phabricator via cfe-commits]

rnk marked an inline comment as done.
rnk added a comment.

Thanks!




Comment at: clang/include/clang/AST/Attrs.h:1
+//===--- Attr.h - Classes for representing attributes --*- C++ -*-===//
+//

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.


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

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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

Basically LGTM but I am hoping we can get a better name than `Attr.h` and 
`Attrs.h`.




Comment at: clang/include/clang/AST/Attrs.h:1
+//===--- Attr.h - Classes for representing attributes --*- C++ -*-===//
+//

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



Comment at: clang/include/clang/AST/Attrs.h:13
+
+#ifndef LLVM_CLANG_AST_ATTRS_H
+#define LLVM_CLANG_AST_ATTRS_H

Once the file name is updated, be sure to update the include guards.


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

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

[Reid Kleckner via Phabricator via cfe-commits]

rnk created this revision.
rnk added a reviewer: aaron.ballman.
Herald added subscribers: arphaman, mgrang, kosarev, jholewinski.
Herald added a reviewer: jdoerfert.
Herald added a project: clang.

Many AST headers require Attr to be complete, but do not need the
complete table generated list of attributes. Separate that into Attrs.h
from Attr.h, and include it where necessary.

-ftime-trace shows that including Attrs.h by itself takes 370ms to
parse, so separating it out is valuable.

After this change, 1706 files depend on Attr.h, but only 600 object
files depend on Attrs.h. This represents a potential savings of 409s CPU
time per build, but on my workstation, a local build only got faster by
2-3s overall (4m45s -> 4m42s).


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D70627

Files:
  clang-tools-extra/clang-tidy/bugprone/VirtualNearMissCheck.cpp
  clang-tools-extra/clang-tidy/hicpp/NoAssemblerCheck.cpp
  clang-tools-extra/clang-tidy/modernize/UseOverrideCheck.cpp
  clang/include/clang/AST/Attr.h
  clang/include/clang/AST/AttrVisitor.h
  clang/include/clang/AST/Attrs.h
  clang/include/clang/AST/RecursiveASTVisitor.h
  clang/include/clang/ASTMatchers/ASTMatchers.h
  clang/include/clang/Sema/Initialization.h
  clang/include/clang/Sema/Sema.h
  clang/include/clang/StaticAnalyzer/Checkers/SValExplainer.h
  clang/lib/AST/AttrImpl.cpp
  clang/lib/AST/CommentSema.cpp
  clang/lib/AST/Decl.cpp
  clang/lib/AST/DeclBase.cpp
  clang/lib/AST/DeclCXX.cpp
  clang/lib/AST/DeclObjC.cpp
  clang/lib/AST/DeclPrinter.cpp
  clang/lib/AST/Expr.cpp
  clang/lib/AST/ExprCXX.cpp
  clang/lib/AST/ExprConstant.cpp
  clang/lib/AST/ItaniumMangle.cpp
  clang/lib/AST/Mangle.cpp
  clang/lib/AST/MicrosoftCXXABI.cpp
  clang/lib/AST/MicrosoftMangle.cpp
  clang/lib/AST/RecordLayoutBuilder.cpp
  clang/lib/AST/Type.cpp
  clang/lib/AST/TypeLoc.cpp
  clang/lib/Analysis/CFG.cpp
  clang/lib/Analysis/Consumed.cpp
  clang/lib/Analysis/RetainSummaryManager.cpp
  clang/lib/Analysis/ThreadSafety.cpp
  clang/lib/Analysis/ThreadSafetyCommon.cpp
  clang/lib/Analysis/UninitializedValues.cpp
  clang/lib/CodeGen/CGBlocks.cpp
  clang/lib/CodeGen/CGBuiltin.cpp
  clang/lib/CodeGen/CGCXX.cpp
  clang/lib/CodeGen/CGCXXABI.cpp
  clang/lib/CodeGen/CGCall.cpp
  clang/lib/CodeGen/CGClass.cpp
  clang/lib/CodeGen/CGDebugInfo.cpp
  clang/lib/CodeGen/CGDecl.cpp
  clang/lib/CodeGen/CGDeclCXX.cpp
  clang/lib/CodeGen/CGExpr.cpp
  clang/lib/CodeGen/CGExprAgg.cpp
  clang/lib/CodeGen/CGExprConstant.cpp
  clang/lib/CodeGen/CGExprScalar.cpp
  clang/lib/CodeGen/CGLoopInfo.cpp
  clang/lib/CodeGen/CGObjC.cpp
  clang/lib/CodeGen/CGObjCGNU.cpp
  clang/lib/CodeGen/CGObjCMac.cpp
  clang/lib/CodeGen/CGOpenMPRuntime.cpp
  clang/lib/CodeGen/CGOpenMPRuntimeNVPTX.cpp
  clang/lib/CodeGen/CGRecordLayoutBuilder.cpp
  clang/lib/CodeGen/CGStmt.cpp
  clang/lib/CodeGen/CGStmtOpenMP.cpp
  clang/lib/CodeGen/CGVTables.cpp
  clang/lib/CodeGen/CodeGenFunction.cpp
  clang/lib/CodeGen/CodeGenFunction.h
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/lib/CodeGen/CodeGenModule.h
  clang/lib/CodeGen/CodeGenTBAA.cpp
  clang/lib/CodeGen/ItaniumCXXABI.cpp
  clang/lib/CodeGen/MicrosoftCXXABI.cpp
  clang/lib/CodeGen/SanitizerMetadata.cpp
  clang/lib/CodeGen/TargetInfo.cpp
  clang/lib/Frontend/Rewrite/RewriteModernObjC.cpp
  clang/lib/Frontend/Rewrite/RewriteObjC.cpp
  clang/lib/Index/CommentToXML.cpp
  clang/lib/Index/IndexDecl.cpp
  clang/lib/Index/IndexSymbol.cpp
  clang/lib/Index/IndexingContext.cpp
  clang/lib/Index/USRGeneration.cpp
  clang/lib/Serialization/ASTReaderDecl.cpp
  clang/lib/Serialization/ASTWriterDecl.cpp
  clang/lib/StaticAnalyzer/Checkers/CheckObjCDealloc.cpp
  clang/lib/StaticAnalyzer/Checkers/DirectIvarAssignment.cpp
  clang/lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/InterCheckerAPI.h
  clang/lib/StaticAnalyzer/Checkers/IvarInvalidationChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/MIGChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/MacOSXAPIChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/MoveChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/NoReturnFunctionChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/NonNullParamChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/ObjCUnusedIVarsChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.cpp
  
clang/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountDiagnostics.cpp
  clang/lib/StaticAnalyzer/Checkers/UndefCapturedBlockVarChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/VirtualCallChecker.cpp
  clang/lib/StaticAnalyzer/Core/CallEvent.cpp
  clang/lib/StaticAnalyzer/Core/MemRegion.cpp
  clang/lib/StaticAnalyzer/Core/RegionStore.cpp
  clang/tools/libclang/CIndex.cpp
  clang/tools/libclang/CXIndexDataConsumer.cpp

Index: clang/tools/libclang/CXIndexDataConsumer.cpp
===
--- clang/tools/libclang/CXIndexDataConsumer.cpp
+++ clang/tools/libcla