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

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

In D126642#3547905 <https://reviews.llvm.org/D126642#3547905>, @sdesmalen wrote:

> 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.

Ah, hrmph, yeah, those have to be available anywhere that a function type is defined, since they are part of the no-prototype type of the function.  So we'd likely need to extract that at the "FunctionType" level, and put it into trailing storage in BOTH places (that is, FunctionNoProtoType would have trailing storage for it if necessary, and FunctionProtoType would as well).  ExtProtoInfo doesn't exist for FunctionNoProtoType because it is C++/parameter-specific stuff (prototypeless function types aren't permitted in C++).

>>> 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!




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126642