aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.
LGTM, though please give other active reviewers a bit of time to chime in
before landing. Note, @erichkeane is out on sabbatical at the moment, so
there's no need to wait for his LG (he can leave post-commit comments on the
review when he gets back though, so keep your eyes peeled for that just in
case).
================
Comment at: clang/include/clang/AST/Type.h:3987
/// [implimits] 8 bits would be enough here.
- uint16_t NumExceptionType = 0;
+ uint16_t NumExceptionType;
+
----------------
sdesmalen wrote:
> aaron.ballman wrote:
> > I think this should also be an `unsigned` bit-field so that it packs
> > together with the new field added below. Otherwise we're liable to end up
> > with padding between this field and the next bit-field (which will take a
> > full allocation unit anyway). How about `unsigned NumExceptionType : 10;`?
> > We're reducing the count by 6 bits, but.... I struggle to imagine code
> > being impacted and we're still allowing more than the implementation limits
> > require.
> That's what I did initially in the patch where I limited `NumExceptionType`
> to 16bits (D152140), but there was the preference to have this be a uint16_t
> instead. I've changed it back again to a bitfield though.
I'm sorry for the back and forth! I think Erich's suggestion was wrong in the
other thread, mostly because of MSVC: https://godbolt.org/z/W9fn7Ghxo
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D127762/new/
https://reviews.llvm.org/D127762
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
