[PATCH] D126642: [Clang] NFCI: Add a new bit HasExtraBitfields to FunctionType.

[Sander de Smalen via Phabricator via cfe-commits]

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