[PATCH] D50261: [AST] Remove unnecessary indirections in DeclarationNameTable

riccibruno added a comment. Yes it would be nice indeed. IIRC it is only needed for QualType in one of the moved class. This patch is especially nice since it both cleanup the code and speed up clang :) Repository: rL LLVM https://reviews.llvm.org/D50261 __

[PATCH] D50261: [AST] Remove unnecessary indirections in DeclarationNameTable

rnk added a comment. Neat, thanks for the optimization. My only concern would be to find a way to avoid including Type.h, but it's already a very popular and very necessary header, so I don't think there is any issue here. Repository: rL LLVM https://reviews.llvm.org/D50261 _

[PATCH] D50261: [AST] Remove unnecessary indirections in DeclarationNameTable

This revision was automatically updated to reflect the committed changes. Closed by commit rL339030: [AST] Remove unnecessary indirections in DeclarationNameTable (authored by brunoricci, committed by ). Herald added a subscriber: llvm-commits. Changed prior to commit: https://reviews.llvm.org/

[PATCH] D50261: [AST] Remove unnecessary indirections in DeclarationNameTable

bricci added a comment. I did both a clean build and an incremental build with and without this patch and was not able to see any meaningful difference in the build time. Suspiciously the build time *with* the patch in both cases is a little less (~0.1% ) than without the patch but this is well

[PATCH] D50261: [AST] Remove unnecessary indirections in DeclarationNameTable

bricci updated this revision to Diff 159044. bricci marked an inline comment as done. bricci added a comment. Added the deleted move ctors. (I think that they were already not declared because of the user-provided copy ctor/assignment) Will report the result about the build time but this will ta

[PATCH] D50261: [AST] Remove unnecessary indirections in DeclarationNameTable

erichkeane added a comment. In https://reviews.llvm.org/D50261#1187705, @bricci wrote: > It seems to me that the increased size of DeclarationNameTable is irrelevant > since it is > only used as a member in ASTContext and never copied nor moved from. Also, > at least for C++ it seems to me tha

[PATCH] D50261: [AST] Remove unnecessary indirections in DeclarationNameTable

bricci added a comment. Additionally I have taken a quick look at the headers which includes DeclarationName.h and it seems that most of them already include Type.h (with the exception of DeclBase.h) If so then the build time shouldn't change much but I will report if there is any significient

[PATCH] D50261: [AST] Remove unnecessary indirections in DeclarationNameTable

bricci added a comment. It seems to me that the increased size of DeclarationNameTable is irrelevant since it is only used as a member in ASTContext and never copied nor moved from. Also, at least for C++ it seems to me that this structure is never "rarely used" since it is used for every overlo

[PATCH] D50261: [AST] Remove unnecessary indirections in DeclarationNameTable

erichkeane added a comment. On the face, this seems to be a nice patch. Removing extra allocations and replacing them with the stack is a good thing IMO. This is clearly an example of PIMPL, which while it has its advantages, performance is typically a hit. My 2 concerns with this are: 1- Th

[PATCH] D50261: [AST] Remove unnecessary indirections in DeclarationNameTable

bricci created this revision. bricci added reviewers: erichkeane, rsmith, rnk. bricci added a project: clang. Herald added a subscriber: cfe-commits. DeclarationNameTable currently hold 3 void * to FoldingSet, FoldingSet and FoldingSet. CXXSpecialName, CXXLiteralOperatorIdName and CXXDeductionGu