[PATCH] D72053: [RFC] Handling implementation limits

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added inline comments.


================
Comment at: clang/include/clang/AST/Decl.h:903
     /// declared.
-    unsigned ScopeDepthOrObjCQuals : NumScopeDepthOrObjCQualsBits;
+    unsigned ScopeDepthOrObjCQuals : IQ::NumScopeDepthOrObjCQualsBits;
 
----------------
These bitfields are assumed to fit into an `unsigned`, and this can no longer be verified locally (changes to the `.td` file can break this assumption). Please add a `static_assert(sizeof(ParmVarDeclBitfields) <= sizeof(unsigned), "...")` next to the `union` below (and likewise for `NonParmVarDeclBitfields`).


================
Comment at: clang/include/clang/AST/Decl.h:903
     /// declared.
-    unsigned ScopeDepthOrObjCQuals : NumScopeDepthOrObjCQualsBits;
+    unsigned ScopeDepthOrObjCQuals : IQ::NumScopeDepthOrObjCQualsBits;
 
----------------
rsmith wrote:
> These bitfields are assumed to fit into an `unsigned`, and this can no longer be verified locally (changes to the `.td` file can break this assumption). Please add a `static_assert(sizeof(ParmVarDeclBitfields) <= sizeof(unsigned), "...")` next to the `union` below (and likewise for `NonParmVarDeclBitfields`).
I think `IQ` is too terse. We don't need a really short name for this; how about `ImpLimits::` or similar?


================
Comment at: clang/include/clang/Basic/ImplementationQuantities.td:107-111
+  bit isBitLimit = 0;
+  bit isValueLimit = 0;
+  bit isCompilerFlagLimit = 0;
+  bit isDescriptionLimit = 0;
+  bit hasRemark = 0;
----------------
Switching between upper- and lower-case names for these fields seems surprising. Please pick a consistent capitalization scheme.


================
Comment at: clang/lib/CodeGen/CGRecordLayout.h:72
   /// The total size of the bit-field, in bits.
-  unsigned Size : 15;
+  unsigned Size : IQ::BitFieldBits;
 
----------------
These three bitfields no longer obviously fit in 32 bits. Please keep the hard-coded 15 here and add an adjacent `static_assert(IQ::BitFieldBits <= 15, "...")` instead, or use some similar strategy so that the bit-field allocation and layout remains locally obvious.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D72053/new/

https://reviews.llvm.org/D72053