[PATCH] D149904: Generic selection expressions that accept a type operand

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman marked 9 inline comments as done.
aaron.ballman added inline comments.


================
Comment at: clang/include/clang/AST/Expr.h:5686
   /// if and only if the generic selection expression is result-dependent.
-  unsigned NumAssocs, ResultIndex;
+  unsigned NumAssocs, ResultIndex, IsExprPredicate;
   enum : unsigned {
----------------
erichkeane wrote:
> I realize this is a rarely used type, but should these be bitfielded somehow?  Particularly now that you're adding a 'bool' field?
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.


================
Comment at: clang/include/clang/AST/Expr.h:5691
 
+  unsigned getIndexOfControllingExpression() const {
+    // If controlled by an expression, the first offset into the Stmt *
----------------
erichkeane wrote:
> What is the value of these functions, if they only return 0 or 1?
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.


================
Comment at: clang/include/clang/AST/Expr.h:5712
+    // predicate expression.
+    return (int)isExprPredicate();
+  }
----------------
erichkeane wrote:
> Should these have asserts also?  Wouldn't saying this is index '0' be incorrect here if t his is the typePredicate?
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).


================
Comment at: clang/include/clang/AST/Expr.h:5930
+  /// argument.
+  bool isExprPredicate() const { return IsExprPredicate; }
+  /// Whether this generic selection uses a type as its controlling argument.
----------------
cor3ntin wrote:
> nitpick: wouldn't `PredicateIsExpr()` make more sense?
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!


================
Comment at: clang/include/clang/AST/Expr.h:5970
   ArrayRef<Expr *> getAssocExprs() const {
     return {reinterpret_cast<Expr *const *>(getTrailingObjects<Stmt *>() +
+                                            getIndexOfStartOfAssociatedExprs()),
----------------
cor3ntin wrote:
> Pre-existing but why not use `cast` here?
This is casting a pointer-to-an-AST-node-pointer, so `cast<>` wouldn't work (that only works on AST node pointers).


================
Comment at: clang/include/clang/Parse/Parser.h:2551
+  bool isTypeIdForGenericSelection() {
+    bool isAmbiguous;
+    if (getLangOpts().CPlusPlus)
----------------
erichkeane wrote:
> 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.
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.


================
Comment at: clang/lib/AST/ASTImporter.cpp:7033-7036
+  if (E->isExprPredicate())
+    ToControllingExpr = importChecked(Err, E->getControllingExpr());
+  else
+    ToControllingType = importChecked(Err, E->getControllingType());
----------------
Fznamznon wrote:
> Just a NIT: would something like
> ```
>   ToControllingExpr = importChecked(Err, E->isExprPredicate()? E->getControllingExpr() : E->getControllingType());
> ```
> look a bit more elegant (when also formatted properly)?
Alas, we can't -- that makes a ternary operator with no common type (`bool ? Expr * : TypeSourceInfo *`)


================
Comment at: clang/lib/Parse/ParseExpr.cpp:3308
+      return ExprError();
+    } else {
+      const auto *LIT = cast<LocInfoType>(ControllingType.get().get());
----------------
Fznamznon wrote:
> No `else` after return?
Good catch!


================
Comment at: clang/lib/Parse/ParseTentative.cpp:645
       isAmbiguous = true;
-
+    // We are supopsed to be inside the first operand to a _Generic selection
+    // expression, so if we find a comma after the declarator, we've found a
----------------
Fznamznon wrote:
> 
Great catch!


================
Comment at: clang/lib/Sema/SemaExpr.cpp:1704-1711
+        // We relax the restriction on use of incomplete types and non-object
+        // types with the type-based extension of _Generic. Allowing incomplete
+        // objects means those can be used as "tags" for a type-safe way to map
+        // to a value. Similarly, matching on function types rather than
+        // function pointer types can be useful. However, the restriction on VM
+        // types makes sense to retain as there are open questions about how
+        // the selection can be made at compile time.
----------------
cor3ntin wrote:
>  Does that mean you can match on array of unknown bound? I struggle to imagine how this could be used.
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.


================
Comment at: clang/lib/Sema/SemaExpr.cpp:19851
+    bool IsExpr = GSE->isExprPredicate();
+    if (IsExpr)
+      ExOrTy = GSE->getControllingExpr();
----------------
erichkeane wrote:
> I find myself wondering if a `getControllingOperand` here is valuable?
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149904