[NFC] [AST] Reduce the size of TemplateParmPosition

[ChuanqiXu]

Summary

I found this when reading the codes. I think it makes sense to reduce the space for TemplateParmPosition. It is hard to image the depth of template parameter is larger than 2^20 and the index is larger than 2^12. So I think the patch might be reasonable.

[aaron.ballman]

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?

[cor3ntin]

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 to do anything, it ought to be a diagnostic?
I can't imagine a scenario in which someone would hit these limits and have assertions enabled. But i agree with you that the limit themselves should not be hit.
On the other hand, why not use 16 for both?

[aaron.ballman]

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 some assertions to the ctor and the setter functions just to be sure though?

If we are going to do anything, it ought to be a diagnostic?

Doing a diagnostic would mean finding all the places where we form a TemplateParmPosition and ensure we have enough source location information to issue the diagnostic. Given that we don't expect users to ever hit it, having the assertion gives a wee bit of coverage (godbolt exposes an assertions build, for example) but without as much implementation burden. That said, if it's easy enough to give diagnostics, that's a more user-friendly approach if we think anyone would hit that limit. (I suspect template instantiation depth would be hit before bumping up against these limits, though.)

I can't imagine a scenario in which someone would hit these limits and have assertions enabled. But i agree with you that the limit themselves should not be hit.
On the other hand, why not use 16 for both?

I think people instantiate to deeper template depths than they typically have for template parameters, so having a deeper depth made sense to me.

[erichkeane]

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 on an integer-sequence of some sort, which would result in depth and position being roughly the same.

[erichkeane]

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

[ChuanqiXu]

Address comments:

• Add assertions to show the input wouldn't hit the boundaries.

[ChuanqiXu]

On the other hand, why not use 16 for both?

I am consistent with @aaron.ballman here.

clang/include/clang/AST/DeclTemplate.h

1155–1166

I don't find standard way to generate the maximum value for bit fields.. So I choose to calculate it by hand.

1162–1163

I find it is possible if ast-dump is going to dump from JSON files.

[aaron.ballman]

clang/include/clang/AST/DeclTemplate.h

1151–1154

Just as constant as a macro, but without all the macro issues.

1155–1166

Since we're adding 1 everywhere we look at these values, would it make more sense to drop the -1 here and the +1 elsewhere?

[ChuanqiXu]

Address comments.

[ChuanqiXu]

clang/include/clang/AST/DeclTemplate.h

1155–1166

Maybe we're able to drop the -1 here. But I feel it breaks the semantics. So I feel it is not good.

[aaron.ballman]

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.

clang/include/clang/AST/DeclTemplate.h

1155–1166

I don't insist. :-)

[ChuanqiXu]

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 we're making a best guess at the bit widths here.

Thanks! I looked at the build bots and the failure cases is about ompt. And I think it is unrelated to this patch though.

clang/include/clang/AST/DeclTemplate.h

 /// Because template parameter can be listed
 /// sequentially for out-of-line template members, each template parameter is
 /// given a Depth - the nesting of template parameter scopes - and a Position -
 /// the occurrence within the parameter list.
 /// This class is inheritedly privately by different kinds of template
 /// parameters and is not part of the Decl hierarchy. Just a facility.
 class TemplateParmPosition {
 protected:
-  // FIXME: These probably don't need to be ints. int:5 for depth, int:8 for
-  // position? Maybe?
-  unsigned Depth;
-  unsigned Position;
-
-  TemplateParmPosition(unsigned D, unsigned P) : Depth(D), Position(P) {}
+  enum { DepthWidth = 20, PositionWidth = 12 };
+  unsigned Depth : DepthWidth;
+  unsigned Position : PositionWidth;
+
+  static constexpr unsigned MaxDepth = (1U << DepthWidth) - 1;
+  static constexpr unsigned MaxPosition = (1U << PositionWidth) - 1;
+
+  TemplateParmPosition(unsigned D, unsigned P) : Depth(D), Position(P) {
+    // The input may fill maximum values to show that it is invalid.
+    // Add one here to convert it to zero.
+    assert((D + 1) <= MaxDepth &&
+           "The depth of template parmeter position is more than 2^20!");
+    assert((P + 1) <= MaxPosition &&
+           "The position of template parmeter position is more than 2^12!");
+  }
 
 public:
   TemplateParmPosition() = delete;
 
   /// Get the nesting depth of the template parameter.
   unsigned getDepth() const { return Depth; }
-  void setDepth(unsigned D) { Depth = D; }
+  void setDepth(unsigned D) {
+    assert((D + 1) <= MaxDepth &&
+           "The depth of template parmeter position is more than 2^20!");
+    Depth = D;
+  }
 
   /// Get the position of the template parameter within its parameter list.
   unsigned getPosition() const { return Position; }
-  void setPosition(unsigned P) { Position = P; }
+  void setPosition(unsigned P) {
+    assert((P + 1) <= MaxPosition &&
+           "The position of template parmeter position is more than 2^12!");
+    Position = P;
+  }
 
   /// Get the index of the template parameter within its parameter list.
   unsigned getIndex() const { return Position; }
 };
 
 /// Declaration of a template type parameter.
 ///
 /// For example, "T" in