Page MenuHomePhabricator

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

Authored by sdesmalen on May 30 2022, 5:15 AM.

Details

Summary

The FunctionTypeExtraBitfields is currently only available when the
ExceptionSpecificationType == Dynamic, which means that there is no other
way to use or extend the FunctionTypeExtraBitfields independently of the
exception specification type.

This patch adds a new field HasExtraBitfields to specify
whether the prototype has trailing ExtraBitfields.

This patch intends to be NFC and is required for future extension
and use of the ExtraBitfields struct.

Diff Detail

Event Timeline

sdesmalen created this revision.May 30 2022, 5:15 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 30 2022, 5:15 AM
sdesmalen requested review of this revision.May 30 2022, 5:15 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 30 2022, 5:15 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript

This patch follows from some discussion on D124998 as well as the need for more bits to represent future function type attributes :)

I'm not sure I'm grokking hte difference between the ExtraBitfields and ExtParamInfos here. Also, the 'hasBitfields' seems like the answer should just be 'no' in the case when its 'no'...

clang/include/clang/AST/Type.h
4099–4103

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?

clang/lib/AST/Type.cpp
3223

this else should have braces, since the 'if' above does. (nit).

Thanks for this! I'm still thinking about the design for this, but spotted a few minor things that could be addressed in the meantime.

clang/include/clang/AST/Type.h
3803–3804

It looks like this comment is now out of date.

3812

More forward-looking given that NumExceptionType really should be an 8-bit wide bit-field.

I'm not sure I'm grokking hte difference between the ExtraBitfields and ExtParamInfos here.

The reason I repurposed the bit was because I previously tried adding a bit to unsigned ExtInfo : 13; but that fails

static_assert(sizeof(*this) <= 8 + sizeof(ExtQualsTypeCommonBase), "changing bitfields changed sizeof(Type)!");

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?

clang/include/clang/AST/Type.h
4099–4103

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'm not sure I'm grokking hte difference between the ExtraBitfields and ExtParamInfos here.

The reason I repurposed the bit was because I previously tried adding a bit to unsigned ExtInfo : 13; but that fails

static_assert(sizeof(*this) <= 8 + sizeof(ExtQualsTypeCommonBase), "changing bitfields changed sizeof(Type)!");

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.

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.

sdesmalen marked 4 inline comments as done.
sdesmalen retitled this revision from [Clang] NFCI: Repurpose HasExtParameterInfos for HasExtraBitfields to [Clang] NFCI: Add a new bit HasExtraBitfields to FunctionType..
sdesmalen edited the summary of this revision. (Show Details)

Instead of repurposing the HasExtParameterInfos bit, added an extra field for HasExtraBitfields.

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!

clang/include/clang/AST/Type.h
3803–3804

Good spot, thanks!

4099–4103

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.

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!

erichkeane accepted this revision.May 31 2022, 11:14 AM

I think I'm Ok with this as a 1st step for the cleanup. I think we should probably evaluate what amount of work is necessary to extract the ExtInfo out into trailing storage, depending on whether it saves us room in the FunctionProtoType and FunctionNoProtoType types.

clang/include/clang/AST/Type.h
4099–4103

Ah, that makes sense, thanks.

This revision is now accepted and ready to land.May 31 2022, 11:14 AM
aaron.ballman accepted this revision.May 31 2022, 12:06 PM

LGTM aside from a nit.

clang/include/clang/AST/Type.h
3809–3810
This revision was landed with ongoing or failed builds.Jun 1 2022, 3:41 AM
This revision was automatically updated to reflect the committed changes.

Thanks for the review and comments @erichkeane and @aaron.ballman!