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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
