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
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
