[PATCH] D50713: [AST] Pack the unsigned of SubstTemplateTypeParmPackType into Type

[Bruno Ricci via Phabricator via cfe-commits]

riccibruno added inline comments.


================
Comment at: include/clang/AST/Type.h:1634
+
+    /// The number of template arguments in \c Arguments.
+    /// Intentionally not a bitfield since we have plenty of space left.
----------------
erichkeane wrote:
> I would love it if we commented what a reasonable number for this is.  Additionally, we should probably make it 64 - NumTypeBits or something, to ensure that if NumTypeBits gets too large that we don't blow 64 bits here as well as communicate that it isn't necessary to keep this 32 bits.
The reason I did not do something like this is that
there is a penalty to access bit-fields due to the various
shifts and masks. IIRC I was actually able to measure it after
I went over each of these classes and only kept bit-fields
when strictly necessary. (however it is hard to be sure
since it was close to the noise of my measurement, and the
difference could be from totally unrelated sources).
In any case it is quite small (maybe ~0.1% in total, not just
from this single case) and IMHO not worth systematically
bothering about it but in this case why pay the penalty if
we can avoid it.

If `NumTypeBits` gets too large then this will be detected by the
the static_asserts and the person who did the modification will
have to convert `NumArgs` to a bit-field. In any case this will at least
also blow up `FunctionTypeBitfields`, `VectorTypeBitfields`,
`AttributedTypeBitfields`, `SubstTemplateTypeParmPackTypeBitfields`,
`TemplateSpecializationTypeBitfields`,
`DependentTemplateSpecializationTypeBitfields`,
and `PackExpansionTypeBitfields`.

Also I do not think that doing
`unsigned NumArgs : 64 - NumTypeBits;` will work because
since `NumTypeBits == 18` this will be
`unsigned NumArgs : 46;` and this will not pack nicely.

Given how many things will not work when `NumTypeBits > 32`
It seems to be that it is not worth converting it to a bit-field.
However a comment indicating how many bits are actually needed
would definitely be a good addition IMO. On this point I have
absolutely no idea and probably @rsmith should comment on this.


Repository:
  rC Clang

https://reviews.llvm.org/D50713