Page MenuHomePhabricator

[C2x] implement typeof and typeof_unqual
ClosedPublic

Authored by aaron.ballman on Sep 20 2022, 8:53 AM.

Details

Summary

This implements WG14 N2927 and WG14 N2930, which together define the feature for typeof and typeof_unqual, which get the type of their argument as either fully qualified or fully unqualified. The argument to either operator is either a type name or an expression. If given a type name, the type information is pulled directly from the given name. If given an expression, the type information is pulled from the expression. Recursive use of these operators is allowed and has the expected behavior (the innermost operator is resolved to a type, and that's used to resolve the next layer of typeof specifier, until a fully resolved type is determined.

Note, we already supported typeof in GNU mode as a non-conforming extension and we are *not* exposing typeof_unqual as a non-conforming extension in that mode, nor are we exposing typeof or typeof_unqual as a nonconforming extension in other language modes. The GNU variant of typeof supports a form where the parentheses are elided from the operator when given an expression (e.g., typeof 0 i = 12;). When in C2x mode, we do not support this extension.

Diff Detail

Event Timeline

aaron.ballman created this revision.Sep 20 2022, 8:53 AM
Herald added a project: Restricted Project. · View Herald Transcript
Herald added a subscriber: martong. · View Herald Transcript
aaron.ballman requested review of this revision.Sep 20 2022, 8:53 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 20 2022, 8:53 AM

Updated the C status page accordingly.

I'm on the fence about re-using the same nodes for typeof and typeof_unqual. However, I have a preference to stop shuffling a bool around everywhere as parameters, and would like an enum (even if we store it as a bitfield : 1). Something like:

enum class TypeofKind {
       Typeof, TypeofUnqual };

I think improves readability in a bunch of places, AND gives us less work to do if we need to extend this node in the future.

clang/include/clang/AST/ASTContext.h
1718

Mild preference for an enum, instead of a bool here.

Alternatively, depending on your design below that I haven't seen yet, perhaps unqual versions of these functions?

clang/include/clang/AST/Type.h
4542

This constructor is smelly.... Am I to guess this does something like, "if E, type is the type of this, else it is the type passed in Can?"

4592

Are TypedefType's EVER canonical? Should you just be asserting 'Can' is canonical?

clang/lib/AST/ASTContext.cpp
12910

I'm unfamiliar with this function, but I would expect you MIGHT need to? If only because they are the same AST node. Should 'unqual' version be its own node? I'm on the fence, as it is a LOT of code to do so, but also ends up being simpler in many places.

clang/lib/AST/Type.cpp
3502

In what situation is getTypePtrOrNull required here? I would imagine that the 'isNull' check above should make sure we aren't in a situation where 'Ret' could be 'null' here? Same with the 'if_present'.

clang/lib/Lex/Preprocessor.cpp
61 ↗(On Diff #461583)

Oh?

clang/lib/Parse/ParseDecl.cpp
7463

Should it be an error to !HasParens if IsUnqual.

clang/lib/Sema/SemaType.cpp
9137

(IsUnqual + 2) maybe? Or perhaps too cute.

shafik added inline comments.Sep 20 2022, 10:15 AM
clang/include/clang/AST/ASTContext.h
1717

I guess these are not purely extensions anymore?

clang/include/clang/AST/Type.h
1071

I think your trying to say this is more than the unqualified type but maybe just be more explicit like it returns the ununqualified type including atomic type qualification and type attributes which behave like a qualifier, such as an address space attribute

4601

It is interesting that you do getTypeofUnqualType in the constructor and then you have to do it again when desuguring. Is this tested in the tests?

mizvekov added inline comments.
clang/include/clang/AST/Type.h
4537

You can use the Type Bitfields in order to avoid bumping the size of the node.

clang/lib/AST/ASTContext.cpp
12910–12916

A qualified and an unqualified typeof could have the same underlying type, so this assert can trip.

I think what makes most sense is to unify them to a qualified typeof in case they differ, as that holds the underlying type unchanged:

return Ctx.getTypeOfType(Ctx.getQualifiedType(Underlying),
                         cast<TypeOfType>(X)->isUnqual() && cast<TypeOfType>(Y)->isUnqual());
mizvekov added inline comments.Sep 20 2022, 10:39 AM
clang/include/clang/AST/Type.h
4601

The constructor takes the canonical type, while this preserves the unmodified type (which can be accessed through getUnderlyingType).

mizvekov added inline comments.Sep 20 2022, 10:46 AM
clang/include/clang/AST/Type.h
4592

They can't, the original assert didn't make much sense.

Though asserting on CanonicalType being canonical is a job better left to the Type constructor, just so we don't have to repeat it everywhere.

mizvekov added inline comments.Sep 20 2022, 10:59 AM
clang/lib/AST/ASTContext.cpp
12910–12916

By the way, one thing to notice is the confusion regarding what is the underlying type of this node, the result of desugar() or the result of getUnderlyingType()?

On my previous post, I meant the former.

Maybe this is a good reason to rename getUnderlyingType() to getUnmodifiedType() or something similar?

aaron.ballman marked 13 inline comments as done.Sep 20 2022, 1:15 PM

I'm on the fence about re-using the same nodes for typeof and typeof_unqual. However, I have a preference to stop shuffling a bool around everywhere as parameters, and would like an enum (even if we store it as a bitfield : 1). Something like:

enum class TypeofKind {
       Typeof, TypeofUnqual };

I think improves readability in a bunch of places, AND gives us less work to do if we need to extend this node in the future.

I'm happy to switch to an enum for this. I don't think it makes a whole lot of sense to add additional types for typeof_unqual support though. Using inheritance would be a bit risky (someone may accidentally use a typeof_unqual type as a typeof), and there's only one bit of difference between the two types.

clang/include/clang/AST/ASTContext.h
1717

Good catch!

clang/include/clang/AST/Type.h
4537

Good call!

4542

No, it's more that E holds the type and Can holds the canonical type, which can sometimes be different than the type of E in the case of dependent typeof expressions. See ASTContext::getTypeOfExprType() for the interesting use.

4601

Yup, this is covered in the tests. desugar() gets called as part of type merging, and the canonical type is used when printing aka information.

clang/lib/AST/Type.cpp
3502

This was a hold over from before I had the isNull() check above, so I've fixed this up.

clang/lib/Lex/Preprocessor.cpp
61 ↗(On Diff #461583)

Wth? No idea how that got here, but it's gone now.

clang/lib/Parse/ParseDecl.cpp
7463

I don't think we can get here in C2x mode, and typeof_unqual is not exposed outside of that mode. We've got test coverage for this (in C2x):

typeof 0 int i = 12;         // expected-error {{expected '(' after 'typeof'}} expected-error {{expected identifier or '('}}
typeof 0 j = 12;             // expected-error {{expected '(' after 'typeof'}} expected-error {{expected identifier or '('}}
typeof_unqual 0 k = 12;      // expected-error {{expected '(' after 'typeof_unqual'}} expected-error {{expected identifier or '('}}
typeof_unqual 0 int l = 12;  // expected-error {{expected '(' after 'typeof_unqual'}} expected-error {{expected identifier or '('}}
clang/lib/Sema/SemaType.cpp
9137

LoL far too cute, but this will change now that I'm using an enum more.

aaron.ballman marked 8 inline comments as done.

Addressing review feedback.

Really update, don't just kinda sorta update.

clang/lib/AST/ASTContext.cpp
12910–12916

FWIW: I still have to address these comments in the patch, but I'll do that tomorrow.

erichkeane added inline comments.Sep 20 2022, 1:39 PM
clang/include/clang/AST/Type.h
1902

So the downside to doing the bitfields is that EVERY 'Type' pays for them, or at least, pays for the largest one. As this is a rarely used and 'leaf' AST node, I would say using the bitfields for this is at minimum 'unnecessary'.

That said, I doubt it is the 'biggest one', so I could go either way.

clang/lib/AST/Type.cpp
3474

Does this have to be defensive about Can being the 'default' value of an empty qual-type?

aaron.ballman marked 4 inline comments as done.Sep 21 2022, 5:34 AM
aaron.ballman added inline comments.
clang/include/clang/AST/Type.h
1902

This is a union of objects, I'm certain all of them are at least one bit wide, so we're not adding to any extra overhead, we're taking advantage of the overhead we're already paying for.

clang/lib/AST/ASTContext.cpp
12910–12916

Thanks @mizvekov -- that makes sense to me. I've changed the implementation here, and I've also renamed getUnderlyingType() to getUnmodifiedType(). I was hesitant to do the rename before (but I was thinking about it!) because all of the "this type represents another type" types use getUnderlyingType() (such as DecltypeType, UnaryTransformType, etc). But in this case, the "underlying type" is less clear because of the unqual variant, so I think renaming is a good idea here.

clang/lib/AST/Type.cpp
3474

getTypeofUnqualType() looks for a null type and handles it appropriately already. If Can isn't valid, then we pass along the invalid type in either branch.

aaron.ballman marked 2 inline comments as done.

Updating based on review feedback; I believe this handles all of the comments.

mizvekov added inline comments.Sep 21 2022, 6:05 AM
clang/lib/AST/Type.cpp
3504–3507

I think this block of code is not actually needed.

So the AttributedType node is just sugar right, the address space are real qualifiers applied to the type, just like const and such.

So the getAtomicUnqualifiedType() alone should do that, as it uses getUnqualifiedType() on the final result.

When unqualifying a type, this will already strip all top level sugar nodes necessary to get rid of the qualifiers.

erichkeane added inline comments.Sep 21 2022, 6:09 AM
clang/include/clang/AST/Type.h
1902

Yep, this was a 'general lament'. Sorry for not being more clear.

aaron.ballman marked an inline comment as done.Sep 21 2022, 6:28 AM
aaron.ballman added inline comments.
clang/lib/AST/Type.cpp
3504–3507

The situation I had in mind for this was something like:

typedef __attribute__((address_space(1))) int foo;
_Atomic foo i;
typeof_unqual(i) j; // Should be int

but I didn't realize we distribute the qualifiers from the inner type to the atomic type: https://godbolt.org/z/bbafvTKsj

So I think you're correct -- we don't seem to need this function at all. (Function type attributes like calling conventions, etc aren't handled via an AttributedType either, but are on the ExtInfo object for the function type.)

Good catch!

aaron.ballman marked an inline comment as done.

Simplifying based on review feedback.

This revision is now accepted and ready to land.Sep 28 2022, 9:47 AM
This revision was landed with ongoing or failed builds.Sep 28 2022, 10:28 AM
This revision was automatically updated to reflect the committed changes.
mizvekov added inline comments.Oct 4 2022, 7:10 AM
clang/include/clang/AST/Type.h
4564

I only noticed this after rebasing some of my patches, I missed this on the original review (same for TypeOfType)

aaron.ballman added inline comments.Oct 4 2022, 9:15 AM
clang/include/clang/AST/Type.h
4564

Sure, on the whole I think this makes the code cleaner. I've updated in 42ad305bdbb87d567d4d365ec5ede8be4d2a4210, thank you for the suggestion!

mizvekov added inline comments.Oct 4 2022, 9:17 AM
clang/include/clang/AST/Type.h
4564

Thanks!