[PATCH] D72053: [RFC] Handling implementation limits

[Mark de Wever via Phabricator via cfe-commits]

Mordante marked 6 inline comments as done.
Mordante added a comment.

Thanks for the feedback!



================
Comment at: clang/include/clang/AST/Decl.h:903
     /// declared.
-    unsigned ScopeDepthOrObjCQuals : NumScopeDepthOrObjCQualsBits;
+    unsigned ScopeDepthOrObjCQuals : IQ::NumScopeDepthOrObjCQualsBits;
 
----------------
rsmith wrote:
> 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?
I feared ImpLimits would be considered too verbose, but I'll change it in the next revision.


================
Comment at: clang/include/clang/AST/Decl.h:903
     /// declared.
-    unsigned ScopeDepthOrObjCQuals : NumScopeDepthOrObjCQualsBits;
+    unsigned ScopeDepthOrObjCQuals : IQ::NumScopeDepthOrObjCQualsBits;
 
----------------
Mordante wrote:
> rsmith wrote:
> > 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?
> I feared ImpLimits would be considered too verbose, but I'll change it in the next revision.
I'm not too fond to use the `static_assert(sizeof(ParamVarDeclBitFields <= sizeof(unsigned), "...");` here and use a hardcoded value and a  `static_assert` in other places. If it's important to see the size of bit-field directly I prefer the style you suggested later: "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."

I'll give it some more thoughts before updating the proposal.


================
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;
----------------
rsmith wrote:
> Switching between upper- and lower-case names for these fields seems surprising. Please pick a consistent capitalization scheme.
This style is used in other .td files. But I'll switch to capitalize the is/has in the next revision. 


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

https://reviews.llvm.org/D72053