[cfe-commits] [PATCH] C1X: generic selections, static asserts

[Peter Collingbourne]

Hi Doug,

Thanks for the review.  I went ahead and committed r129553-5, taking
into account your comments.

On Sun, Mar 27, 2011 at 01:06:47AM +0100, Douglas Gregor wrote:
> +GenericSelectionExpr::GenericSelectionExpr(ASTContext &Context,
> +                               SourceLocation GenericLoc, Expr *ControllingExpr,
> +                               TypeSourceInfo **_AssocTypes, Expr **AssocExprs,
> +                               unsigned NumAssocs, SourceLocation DefaultLoc,
> +                               SourceLocation RParenLoc)
> +  : Expr(GenericSelectionExprClass,
> +         Context.DependentTy,
> +         VK_RValue,
> +         OK_Ordinary,
> +         /*isTypeDependent=*/  true,
> +         /*isValueDependent=*/ true,
> +         /*containsUnexpandedParameterPack=*/ false),
> 
> This last bit is actually interesting... do we check for unexpanded parameter packs in Sema? It doesn't look like we do.

No, and we should.  Now we check for unexpanded parameter packs
in the controlling expression and all association types/expressions
(regardless of whether they are in the result expression's association;
this is consistent with the lexical definition of a pattern expansion
given in [temp.variadic]p4).

> +  SourceLocation DefaultLoc;
> +  TypeVector Types(Actions);
> +  ExprVector Exprs(Actions);
> +  while (1) {
> +    ParsedType Ty;
> +    if (Tok.is(tok::kw_default)) {
> +      if (!DefaultLoc.isInvalid()) {
> +        Diag(Tok, diag::err_duplicate_default_assoc);
> +        Diag(DefaultLoc, diag::note_previous_default_assoc);
> +        SkipUntil(tok::r_paren);
> +        return ExprError();
> +      }
> +      DefaultLoc = ConsumeToken();
> +      Ty = ParsedType();
> +    } else {
> +      ColonProtectionRAIIObject X(*this);
> +      TypeResult TR = ParseTypeName();
> +      if (TR.isInvalid()) {
> +        SkipUntil(tok::r_paren);
> +        return ExprError();
> +      }
> +      Ty = TR.release();
> +    }
> +    Types.push_back(Ty);
> +
> +    if (ExpectAndConsume(tok::colon, diag::err_expected_colon, "")) {
> +      SkipUntil(tok::r_paren);
> +      return ExprError();
> +    }
> +
> +    ExprResult ER(ParseAssignmentExpression());
> 
> Should we be parsing these expressions in a potentially potentially evaluated context, and only consider the chosen expression to be in a potentially evaluated context?

I think this is the logically correct thing to do, but I don't think
it is worth it right now for the sake of a few diagnostics, as it
would involve a major restructuring of how expression evaluation
contexts work.  I added a FIXME indicating that this is what we should
aim to do.

> 
> --- a/lib/Sema/SemaExpr.cpp
> +++ b/lib/Sema/SemaExpr.cpp
> @@ -701,6 +701,162 @@ QualType Sema::UsualArithmeticConversions(Expr *&lhsExpr, Expr *&rhsExpr,
>  //===----------------------------------------------------------------------===//
>  
>  
> +ExprResult
> +Sema::ActOnGenericSelectionExpr(SourceLocation KeyLoc,
> +                                SourceLocation DefaultLoc,
> +                                SourceLocation RParenLoc,
> +                                Expr *ControllingExpr,
> +                                MultiTypeArg types,
> +                                MultiExprArg exprs) {
> +  unsigned NumAssocs = types.size();
> +  assert(NumAssocs == exprs.size());
> +
> +  ParsedType *ParsedTypes = types.release();
> +  Expr **Exprs = exprs.release();
> +
> +  TypeSourceInfo **Types = new TypeSourceInfo*[NumAssocs];
> +  for (unsigned i = 0; i < NumAssocs; ++i) {
> +    if (ParsedTypes[i])
> +      (void) GetTypeFromParser(ParsedTypes[i], &Types[i]);
> +    else
> +      Types[i] = 0;
> +  }
> +
> 
> If the type ends up being NULL because of an error, we should return an ExprResult(). I guess there could be one NULL ParsedType, for the default case?

If a type is invalid, that will be caught by the parser which will
return ExprError().  The parser also enforces the single default
generic association requirement.

> 
> +      // C1X 6.5.1.1p2 "No two generic associations in the same generic
> +      // selection shall specify compatible types."
> +      for (unsigned j = i+1; j < NumAssocs; ++j)
> +        if (Types[j] && !Types[j]->getType()->isDependentType())
> +          if (Context.typesAreCompatible(Types[i]->getType(),
> +                                         Types[j]->getType())) {
> +            Diag(Types[j]->getTypeLoc().getBeginLoc(),
> +                 diag::err_assoc_compatible_types)
> +              << Types[j]->getTypeLoc().getSourceRange()
> +              << Types[j]->getType()
> +              << Types[i]->getType();
> +            Diag(Types[i]->getTypeLoc().getBeginLoc(),
> +                 diag::note_compat_assoc)
> +              << Types[i]->getTypeLoc().getSourceRange()
> +              << Types[i]->getType();
> +            TypeErrorFound = true;
> +          }
> +    }
> 
> In C++, this could just be SmallPtrSet on the canonical types. Then we could cook up a pointless microbenchmark that shows that compiling C++ is faster than compiling C! 

I guess so, but that would make it harder to issue accurate
diagnostics, so I left it as is.

>  KEYWORD(_Complex                    , KEYALL)
>  KEYWORD(_Generic                    , KEYC1X)
>  KEYWORD(_Imaginary                  , KEYALL)
> +KEYWORD(_Static_assert              , KEYC1X)
>  KEYWORD(__func__                    , KEYALL)
>  
>  // C++ 2.11p1: Keywords.
> 
> Any particular reason why this isn't just an alias for "static_assert"?

_Static_assert is an extension in C++0x, while static_assert is not.
(I made _Static_assert KEYALL and added an extension diagnostic
for it.)

Thanks,
-- 
Peter