[PATCH] D123298: [NFC] [AST] Reduce the size of TemplateParmPosition

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG36de2d639eca: [NFC] [AST] Reduce the size of TemplateParmPosition (authored by ChuanqiXu). Repository: rG LLVM Github Monorepo CHANGES SINCE

[PATCH] D123298: [NFC] [AST] Reduce the size of TemplateParmPosition

ChuanqiXu added a comment. In D123298#3445131 , @aaron.ballman wrote: > Still LG to me, but please watch the build bots and issues list closely for > any new template instantiation depth/position related issues over the next > short while given that

[PATCH] D123298: [NFC] [AST] Reduce the size of TemplateParmPosition

aaron.ballman added a comment. Still LG to me, but please watch the build bots and issues list closely for any new template instantiation depth/position related issues over the next short while given that we're making a best guess at the bit widths here. Comment at:

[PATCH] D123298: [NFC] [AST] Reduce the size of TemplateParmPosition

ChuanqiXu marked an inline comment as done. ChuanqiXu added inline comments. Comment at: clang/include/clang/AST/DeclTemplate.h:1156-1157 - TemplateParmPosition(unsigned D, unsigned P) : Depth(D), Position(P) {} + static constexpr unsigned MaxDepth = (1l << DEPTH_BITWIDTH) -

[PATCH] D123298: [NFC] [AST] Reduce the size of TemplateParmPosition

ChuanqiXu updated this revision to Diff 422104. ChuanqiXu added a comment. Address comments. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D123298/new/ https://reviews.llvm.org/D123298 Files: clang/include/clang/AST/DeclTemplate.h Index: clang/include/clang/AST/DeclTemplate.h

[PATCH] D123298: [NFC] [AST] Reduce the size of TemplateParmPosition

aaron.ballman added inline comments. Comment at: clang/include/clang/AST/DeclTemplate.h:1151-1152 protected: - // FIXME: These probably don't need to be ints. int:5 for depth, int:8 for - // position? Maybe? - unsigned Depth; - unsigned Position; +#define DEPTH_BITWIDTH 20

[PATCH] D123298: [NFC] [AST] Reduce the size of TemplateParmPosition

ChuanqiXu added a comment. > On the other hand, why not use 16 for both? I am consistent with @aaron.ballman here. Comment at: clang/include/clang/AST/DeclTemplate.h:1156-1157 - TemplateParmPosition(unsigned D, unsigned P) : Depth(D), Position(P) {} + static constexpr

[PATCH] D123298: [NFC] [AST] Reduce the size of TemplateParmPosition

ChuanqiXu updated this revision to Diff 421419. ChuanqiXu added a comment. Address comments: - Add assertions to show the input wouldn't hit the boundaries. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D123298/new/ https://reviews.llvm.org/D123298 Files:

[PATCH] D123298: [NFC] [AST] Reduce the size of TemplateParmPosition

erichkeane added a comment. I note that 'recursive' depth is limited to 1024: https://godbolt.org/z/h9bEfsToT Though I think there are other ways to get 'deeper' than that. So perhaps in any case we're just arranging deck chairs on a -post-error titanic :) Repository: rG LLVM Github

[PATCH] D123298: [NFC] [AST] Reduce the size of TemplateParmPosition

erichkeane added a comment. > I think people instantiate to deeper template depths than they typically have > for template parameters, so having a deeper depth made sense to me. Hmm... the cases I can think of make me think this isn't necessarily true... It is very common to template-recurse

Re: [PATCH] D123298: [NFC] [AST] Reduce the size of TemplateParmPosition

On Thu, Apr 7, 2022 at 8:35 AM Corentin via cfe-commits wrote: > > > > On Thu, Apr 7, 2022 at 2:11 PM Aaron Ballman via Phabricator > wrote: >> >> aaron.ballman added a comment. >> >> In D123298#3435796 , @cor3ntin >> wrote: >> >> > In D123298#3435770

Re: [PATCH] D123298: [NFC] [AST] Reduce the size of TemplateParmPosition

On Thu, Apr 7, 2022 at 2:11 PM Aaron Ballman via Phabricator < revi...@reviews.llvm.org> wrote: > aaron.ballman added a comment. > > In D123298#3435796 , @cor3ntin > wrote: > > > In D123298#3435770 , >

[PATCH] D123298: [NFC] [AST] Reduce the size of TemplateParmPosition

aaron.ballman added a comment. In D123298#3435796 , @cor3ntin wrote: > In D123298#3435770 , @aaron.ballman > wrote: > >> Changes LGTM, I also don't think we should hit these limits. Perhaps we >> should add

[PATCH] D123298: [NFC] [AST] Reduce the size of TemplateParmPosition

cor3ntin added a comment. In D123298#3435770 , @aaron.ballman wrote: > Changes LGTM, I also don't think we should hit these limits. Perhaps we > should add some assertions to the ctor and the setter functions just to be > sure though? If we are going

[PATCH] D123298: [NFC] [AST] Reduce the size of TemplateParmPosition

aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land. Changes LGTM, I also don't think we should hit these limits. Perhaps we should add some assertions to the ctor and the setter functions just to be sure though? Repository: rG

[PATCH] D123298: [NFC] [AST] Reduce the size of TemplateParmPosition

ChuanqiXu created this revision. ChuanqiXu added reviewers: erichkeane, aaron.ballman, cor3ntin. ChuanqiXu added a project: clang. Herald added a subscriber: kristof.beyls. Herald added a project: All. ChuanqiXu requested review of this revision. Herald added a subscriber: cfe-commits. I found