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