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

[Erich Keane via Phabricator via cfe-commits]

erichkeane 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.
----------------
riccibruno wrote:
> 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.
My biggest fear here is basically that a future programmer coming in is going to figure that NumArgs REQUIRES 32 bits and try to start the discussion as to whether we can blow this bitfields up.

A comment saying most of what you said above, plus a "This corresponds to implimits <whatever>, so it needs to support at least <num-ever>.


Repository:
  rC Clang

https://reviews.llvm.org/D50713