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- The increased size of DeclarationNameTable will cause a performance regression in the case where these folding sets are used rarely. Copies (which are obviously deleted, unless memcpy'ed somewhere)/moves of DeclarationNameTable are now more expensive. Based on your analysis, it seems that this is a 'net win', but I'd like to see if we can disallow 'move' as well. 2- The advantage of PIMPL is a reduction in compile time. Now, every file that uses DeclarationName.h (an extensive list of .cpp files, but more importantly, Decl.h, DeclBase.h, and Sema.h). Could you perhaps do a clean build both before/after this change with 'time' (http://man7.org/linux/man-pages/man1/time.1.html) and show the results? I hope it ends up being a small enough change to be acceptable, but it would be nice to have an informed decision here. ================ Comment at: include/clang/AST/DeclarationName.h:450 DeclarationNameTable &operator=(const DeclarationNameTable &) = delete; /// getIdentifier - Create a declaration name that is a simple ---------------- DeclarationNameTable should definitely have the move operators dealt with here. I suspect/hope the answer is 'deleted', but I don't think it matters generally. Additionally, I'd like to see the destructor line left and set to 'default', since those 2 would complete the 'rule of 5'. Repository: rC Clang https://reviews.llvm.org/D50261 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
