ChuanqiXu accepted this revision. ChuanqiXu added a comment. This revision is now accepted and ready to land.
In D126907#3619270 <https://reviews.llvm.org/D126907#3619270>, @erichkeane wrote: > All but the 1 comment from @ChuanqiXu fixed, not sure what to do about the > 'info'. LGTM. But the 'info' comment doesn't matter a lot. The current complexity is acceptable and its semantics is stated clearly in the comments. My intention is to make its semantics more clear and more readable so that it is easier to maintain and change it. But this is required. Since the patch is really large. So my preference is to do other unnecessary changes in other revision to make the patch easier to read. (Just a preference. I know some people prefer large patches.) ================ Comment at: clang/include/clang/AST/Decl.h:1944-1948 + /// For non-templates this value will be NULL, unless this non-template + /// function declaration was declared directly inside of a function template, + /// in which case this will have a pointer to a FunctionDecl, stored in the + /// NamedDecl. For function declarations that describe a function template, + /// this will be a pointer to a FunctionTemplateDecl, stored in the NamedDecl. ---------------- erichkeane wrote: > ChuanqiXu wrote: > > Could we give a new abstract like `XXXInfo` to replace `NamedDecl*` here? > > The NamedDecl* covers a lot of things. It looks more consistent. > All of those 'Info' types contain significantly more information, which we > really don't have a need for here. Basically all this patch is doing is > using the FunctionTemplateDecl* that was already in the list and generalizing > it a bit. AND unfortunately, the only commonality between > FunctionTemplateDecl and FunctionDecl is NamedDecl. > > So any type we would use would end up causing an additional allocation here, > and make its use require us to unpack it :/ > > I guess what I'm saying, is I'm not sure what that would look like in a way > that wouldn't make this worse. Do you have an idea you could share that > would improve that? > > My idea would be something like: ```C++ llvm::PointerUnion<FunctionDecl *, FunctionTemplateDecl*, MemberSpecializationInfo *, FunctionTemplateSpecializationInfo *, ``` > So any type we would use would end up causing an additional allocation here, > and make its use require us to unpack it :/ This is a union so it wouldn't cause additional allocations? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D126907/new/ https://reviews.llvm.org/D126907 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
