This is an archive of the discontinued LLVM Phabricator instance.

Generic selection expressions that accept a type operand
ClosedPublic

Authored by aaron.ballman on May 4 2023, 2:08 PM.

Details

Summary

Extends _Generic selection expressions to accept a type operand in addition to an expression operand. The type operand form does not perform lvalue conversions on the operand, which allows the expression to select *qualified* types, making it more useful for generic programming in C.

C has a few operators that take either a type or an expression, such as sizeof. It is natural to extend that idea to _Generic so that it can also accept a type for the first operand. This type does not undergo any conversions, which allows it to match qualified types, incomplete types, and function types. C2x has the typeof operator as a way to get the type of an expression before lvalue conversion takes place, and so it keeps the qualification. This makes typeof a straightforward approach to determining a type operand for _Generic that considers qualifiers. This allows writing a helper macro like:

#define EXPR_HAS_TYPE(Expr, Type) _Generic(typeof(Expr), Type : 1, default : 0)

which can be called with an expression of any value category (no need to be an lvalue) and will test against (almost) any type. This is a conforming extension to C (it's defining the behavior of invalid syntax), and I believe this is a natural way to extend this functionality.

_Generic with a type operand will relax the requirements of what can be a valid association. Specifically, it allows incomplete types and non-object types (but still prevents use of variably-modified types). This relaxation only happens for the type operand form; the expression operand form continues to behave as it always has.

This extension allows incomplete and non-object types because the goal is to better enable type-generic programming in C, and so we want to allow any typed construct we can statically determine the type of. There is no reason to prevent matching against void or function types, but this does explain why we continue to prohibit variably-modified types.

Please see the RFC at https://discourse.llvm.org/t/rfc-generic-selection-expression-with-a-type-operand/70388 for more details.

Diff Detail

Unit TestsFailed

Event Timeline

aaron.ballman created this revision.May 4 2023, 2:08 PM
Herald added a reviewer: NoQ. · View Herald Transcript
Herald added a project: Restricted Project. · View Herald Transcript
Herald added a subscriber: martong. · View Herald Transcript
aaron.ballman requested review of this revision.May 4 2023, 2:08 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 4 2023, 2:08 PM
erichkeane added inline comments.
clang/include/clang/AST/Expr.h
5686

I realize this is a rarely used type, but should these be bitfielded somehow? Particularly now that you're adding a 'bool' field?

5691

What is the value of these functions, if they only return 0 or 1?

5712

Should these have asserts also? Wouldn't saying this is index '0' be incorrect here if t his is the typePredicate?

clang/include/clang/Parse/Parser.h
2551

slight preference to put this declaration in the 'if' condition below, the split in location confused me for a bit trying to figure out where it was being used.

clang/lib/Sema/SemaExpr.cpp
19851

I find myself wondering if a getControllingOperand here is valuable?

I observe there is no documentation or release notes yet :)

clang/include/clang/AST/Expr.h
5930

nitpick: wouldn't PredicateIsExpr() make more sense?

5970

Pre-existing but why not use cast here?

clang/lib/Sema/SemaExpr.cpp
1704–1711

Does that mean you can match on array of unknown bound? I struggle to imagine how this could be used.

Fznamznon added inline comments.
clang/lib/AST/ASTImporter.cpp
7033–7036

Just a NIT: would something like

ToControllingExpr = importChecked(Err, E->isExprPredicate()? E->getControllingExpr() : E->getControllingType());

look a bit more elegant (when also formatted properly)?

clang/lib/Parse/ParseExpr.cpp
3308

No else after return?

clang/lib/Parse/ParseTentative.cpp
645
aaron.ballman marked 9 inline comments as done.May 5 2023, 5:42 AM
aaron.ballman added inline comments.
clang/include/clang/AST/Expr.h
5686

The language which specifies that an implementation need only support 511 identifiers with block scope declared in one block and 127 nesting levels of blocks.... is silent on how many generic selection expression associations we're required to support. :-D

I think we could probably assume there's < 2^15 associations in real world code so we can cram all of this into one 32-bit value.

5691

I was on the fence about these -- it was a nice place to hang an assertion and get a "named constant", basically. I can drop them if you'd like.

5712

Nope, these should not assert -- the selection is required to have at least one association (and an association is a pair of type and expr), so this is getting you the offset to the first association (either the expr or the type).

5930

Hmmm, I think it would be: isPredicateAnExpr() and isPredicateAType() but I'm also not unhappy with the current names (I read them as "is (expression predicate)" and "is (type predicate)"). Your call!

5970

This is casting a pointer-to-an-AST-node-pointer, so cast<> wouldn't work (that only works on AST node pointers).

clang/include/clang/Parse/Parser.h
2551

Sure -- I was just trying to avoid curly braces as done in the next function. :-D I'll fix up the one in isTypeIdUnambiguously() as a drive-by so they remain consistent.

clang/lib/AST/ASTImporter.cpp
7033–7036

Alas, we can't -- that makes a ternary operator with no common type (bool ? Expr * : TypeSourceInfo *)

clang/lib/Parse/ParseExpr.cpp
3308

Good catch!

clang/lib/Parse/ParseTentative.cpp
645

Great catch!

clang/lib/Sema/SemaExpr.cpp
1704–1711

Yup, you can match on an array of unknown bound if you so desire. It's actually useful in an interesting way:

static_assert(_Generic(int[12], int[] : 0, int * : 1) == 0); // OK
static_assert(_Generic(int[12], int[] : 0, int [100] : 1) == 0); // error: type 'int[100]' in generic association compatible with previously specified type 'int[]'

so you can use an incomplete array type as an association and it will match any constant-size array type as a controlling type predicate.

19851

This pattern only comes up once, so I don't think it's worth it, but it's certainly easy enough to add if you disagree.

erichkeane added inline comments.May 5 2023, 5:59 AM
clang/include/clang/AST/Expr.h
5712

OH! I see, there are 2 separate lists here, 1 for types, 1 for expressions, is that it? And you're storing the controlling expression/type in the 1st one (if it exists)?

clang/lib/Sema/SemaExpr.cpp
19851

Yep, I think I agree, after seeing the rest of the review, I don't see as much value.

On this re-read, I kind of which we instead had 2 separate 'create' functions here, rather than going through void*, so that this becomes:

if (IsExpr)
  return AnyChanged ? S.CreateGenericSelectionExpr(...., GSE->getControllingExpr(), ...) : ExprEmpty();

  return AnyChanged ? S.CreateGenericSelectionExpr(...., GSE->getControllingType(), ...) : ExprEmpty();

The void* version gives me the jeebies (and is perhaps a 'worse' interface? Particularly for libclang folks?).

aaron.ballman marked 10 inline comments as done.May 5 2023, 6:04 AM
aaron.ballman added inline comments.
clang/include/clang/AST/Expr.h
5712

Correct! Two *almost* parallel lists where the first entry in either (but not both) list is potentially the controlling operand.

clang/lib/Sema/SemaExpr.cpp
19851

That makes this one use better but at the expense of making that create function rather awkward.

aaron.ballman marked an inline comment as done.

Updated based on review feedback.

I observe there is no documentation or release notes yet :)

Those are coming up, I wanted to see if the RFC made any substantive changes to the design before doing the documentation for the extension.

clang/lib/Sema/SemaExpr.cpp
19851

I took another run at this and decided to put a FIXME comment in the header file. I agree that the interface is a bit awkward, but it's also pretty hard to make work in ActOnGenericSelectionExpr due to it taking an Expr * or a ParsedType (currently it takes a void * of the parsed type's opaque value).

Added a feature test macro, documentation, and a release note.

cor3ntin added inline comments.May 6 2023, 5:41 AM
clang/include/clang/AST/Expr.h
5691

I personally think it's cleaner and more consistent that way, I'd keep them

clang/test/Sema/generic-selection-type-extension.c
131

Can you add a type for something like that?

cpp
template <typename... T>
void f() {
    _Generic(T..., int : 1);
}
aaron.ballman marked 2 inline comments as done.

Added a test case for packs based on review feedback.

erichkeane accepted this revision.Jun 5 2023, 7:44 AM
This revision is now accepted and ready to land.Jun 5 2023, 7:44 AM
This revision was landed with ongoing or failed builds.Jun 5 2023, 8:10 AM
This revision was automatically updated to reflect the committed changes.