sdesmalen added a comment.
In D126642#3547526 <https://reviews.llvm.org/D126642#3547526>, @erichkeane
wrote:
> Right, yeah. One thing to consider: `ExtInfo` was 'first', and so it got to
> keep the 'in bitfield bits'. However, much of what is in there is, IMO, not
> something that is used often enough to be worth having in there. Of the bits
> being used there, I think 'NoReturn' is the only one used with any regularity
> (other than perhaps 'this call' in Windows-32-bit machines). I wonder if we
> should consider moving as much of that as possible over.
That sounds sensible. Some of the fields there are also valid in
FunctionNoProtoType though, and I don't believe I saw an equivalent to
ExtProtoInfo for FunctionNoProtoType, is that because it's uncommon enough that
it only requires changing in a handful of places? I'm not too familiar with
Clang code, so I didn't look to deep into this.
>> so perhaps incorrectly I assumed I couldn't add any new bits to FunctionType
>> and thought I'd repurpose this one bit, because it's only ever used for
>> FunctionProtoType (never for FunctionNoProtoType).
>>
>> But I now realised I can just add an extra bit, so that we can leave this
>> bit as-is. What do you think?
>
> I think there is probably value to that, yes.
Great, thanks, I've updated that now!
================
Comment at: clang/include/clang/AST/Type.h:3801-3802
/// FunctionTypeBitfields. Aligned to alignof(void *) to maintain the
/// alignment of subsequent objects in TrailingObjects. You must update
/// hasExtraBitfields in FunctionProtoType after adding extra data here.
struct alignas(void *) FunctionTypeExtraBitfields {
----------------
aaron.ballman wrote:
> It looks like this comment is now out of date.
Good spot, thanks!
================
Comment at: clang/include/clang/AST/Type.h:4103
bool hasExtraBitfields() const {
- return hasExtraBitfields(getExceptionSpecType());
+ assert((getExceptionSpecType() != EST_Dynamic ||
+ FunctionTypeBits.HasExtraBitfields) &&
----------------
sdesmalen wrote:
> erichkeane wrote:
> > Why is asking if we DO have extra bitfields an assert here? I would think
> > this is a valid question...
> >
> > Why would 'dynamic' and extra-bitfields be exclusive here?
> This assert is merely confirming that HasExtraBitfields **must** be true if
> the ExceptionSpecType is `EST_Dynamic`, because that was the old behaviour
> (and I wanted to avoid introducing failures if some code still assumed that
> hasExtraBitfields == true, but for some reason HasExtraBitfields had not yet
> been set to `true`).
I've marked the comment as done hoping that my above explanation clarified it,
but let me know if you're not happy with it.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D126642/new/
https://reviews.llvm.org/D126642
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
