[Parse] Allow 'constexpr' in condition declarations

[meadori]

Summary

This patch implements the functionality specified by DR948.
The changes are two fold. First, the parser was modified
to allow 'constexpr's to appear in condition declarations
(which was a hard error before). Second, Sema was modified
to cleanup maybe odr-used declarations. As 'constexpr's
were not allowed in condition declarations before the cleanup
wasn't necessary (such declarations were always odr-used).

This fixes PR22491.

[meadori]

Ping.

[rsmith]

Thanks!

Marking a declaration as constexpr should have no effect on whether it results in odr-uses of other things, so I'm surprised that you're seeing a difference there. What's the testcase, and how does it fail?

include/clang/Parse/Parser.h

1760 ↗

(On Diff #23609)

Please add a new DSC value for conditions rather than adding a flag here.

lib/Sema/SemaExprCXX.cpp

2585 ↗

(On Diff #23609)

Do your tests cover the need for this?

test/CXX/stmt.stmt/stmt.select/p6.cpp

4–5 ↗

(On Diff #23609)

The right place for such a test is tests/CXX/drs/dr9xx.cpp.

[meadori]

Opening up in constexpr s in conditions allows "maybe" odr used expression to be collected in cases like:

class T {
public:
   constexpr T(int v) : v(v) { }
   constexpr operator int() const { return v; }
private:
   int v;
};

int main() {
   if (constexpr T x = 200) { }
}

Without the cleanup the following assert fires:

(MaybeODRUseExprs.empty() && "Leftover expressions for odr-use checking"), function ActOnFinishFunctionBody, file /Users/meadori/Code/source/llvm/tools/clang/lib/Sema/SemaDecl.cpp, line 10820.

MaybeODRUseExprs is populated in DoMarkVarDeclReferenced from SemaExpr.cpp (the end of the function does an explicit check for constant expressions). I don't think this situation could ever happen for conditional declarations before, but know it can and we need the new cleanup.

include/clang/Parse/Parser.h

1760 ↗

(On Diff #23609)

Will fix.

lib/Sema/SemaExprCXX.cpp

2585 ↗

(On Diff #23609)

Not yet, but I will add a test for it.

test/CXX/stmt.stmt/stmt.select/p6.cpp

4–5 ↗

(On Diff #23609)

Will fix.

[meadori]

Address review feedback.

[rsmith]

This is looking good.

include/clang/Parse/Parser.h

1689 ↗

(On Diff #24914)

I think this should be called DSC_condition?

lib/Sema/SemaExprCXX.cpp

2586–2589 ↗

(On Diff #24914)

For the switch condition case, additional conversion will be performed on the result of this function; you should delay the ActOnFinishFullExpr until after those conversions in that case.

[meadori]

include/clang/Parse/Parser.h

1689 ↗

(On Diff #24914)

Agreed. Will fix.

lib/Sema/SemaExprCXX.cpp

2586–2589 ↗

(On Diff #24914)

Ah, thank you for pointing that out (again; sorry I missed the suggestion before :-) ). I suppose this new call to ActOnFinishFullExpr can just happen for ConvertToBoolean since ActOnStartOfSwitchStmt already has a call to ActOnFinishFullExpr after the said conversions happen.

This is the easiest thing to do now. After this patch is committed I think there is a potential refactoring to consolidate the check and conversion logic for if, for, switch, and while so that there is only one call to ActOnFinishFullExpr.

[meadori]

Here is a new version with the previous feedback incorporated:

1. Use 'DSC_condition' instead of 'DSC_constexpr'.

2. Delay applying ActOnFinishFullExpr until all switch conditions are done. I ended up unconditionally adding ActOnFinishFullExpr for the if, for, while, and switch cases. There is a small amount of duplication, but it makes the code easier to follow since you don't have to think about under what conditions the call happens (and I personally found it difficult to understand why it was conditional with 'switch' to begin with). Also, it seems to be common practice to directly call ActOnFinishFullExpr in a lot of the other Sema implementation pieces.

[rsmith]

It looks like you uploaded the wrong patch.

[meadori]

Uploaded the wrong patch before.

[meadori]

Ping.

[meadori]

Ping^2.

[rsmith]

test/CXX/drs/dr9xx.cpp

48 ↗

(On Diff #27546)

Add a comment here: // dr948: 3.7 (we have a script that generates the cxx_dr_status.html page from these comments).

50–52 ↗

(On Diff #27546)

Remove this comment: this is implied for all the tests in this entire directory.

cfe/trunk/include/clang/Parse/Parser.h

enum DeclSpecContext {
     DSC_normal, // normal context
     DSC_class,  // class context, enables 'friend'
     DSC_type_specifier, // C++ type-specifier-seq or C specifier-qualifier-list
     DSC_trailing, // C++11 trailing-type-specifier in a trailing return type
     DSC_alias_declaration, // C++11 type-specifier-seq in an alias-declaration
     DSC_top_level, // top-level/namespace declaration context
     DSC_template_type_arg, // template type argument context
     DSC_objc_method_result, // ObjC method result context, enables 'instancetype'
+    DSC_condition // condition declaration context
   };
 
   /// Is this a context in which we are parsing just a type-specifier (or
   /// trailing-type-specifier)?
   static bool isTypeSpecifier(DeclSpecContext DSC) {
     switch (DSC) {
     case DSC_normal:
     case DSC_class:
     case DSC_top_level:
     case DSC_objc_method_result:
+    case DSC_condition:
       return false;
 
     case DSC_template_type_arg:
     case DSC_type_specifier:
     case DSC_trailing:
     case DSC_alias_declaration:
       return true;
     }

cfe/trunk/lib/Parse/ParseDecl.cpp

if (Specs & DeclSpec::PQ_FunctionSpecifier) {
     if (DS.isVirtualSpecified())
       Diag(DS.getVirtualSpecLoc(), diag::err_typename_invalid_functionspec);
     if (DS.isExplicitSpecified())
       Diag(DS.getExplicitSpecLoc(), diag::err_typename_invalid_functionspec);
     DS.ClearFunctionSpecs();
   }
 
   // Issue diagnostic and remove constexpr specfier if present.
-  if (DS.isConstexprSpecified()) {
+  if (DS.isConstexprSpecified() && DSC != DSC_condition) {
     Diag(DS.getConstexprSpecLoc(), diag::err_typename_invalid_constexpr);
     DS.ClearConstexprSpec();
   }
 }
 
 /// isValidAfterIdentifierInDeclaratorAfterDeclSpec - Return true if the
 /// specified token is valid after the identifier in a declarator which
 /// immediately follows the declspec.  For example, these things are valid:

cfe/trunk/lib/Parse/ParseExprCXX.cpp

if (ConvertToBoolean)
       ExprOut
         = Actions.ActOnBooleanCondition(getCurScope(), Loc, ExprOut.get());
     return ExprOut.isInvalid();
   }
 
   // type-specifier-seq
   DeclSpec DS(AttrFactory);
   DS.takeAttributesFrom(attrs);
-  ParseSpecifierQualifierList(DS);
+  ParseSpecifierQualifierList(DS, AS_none, DSC_condition);
 
   // declarator
   Declarator DeclaratorInfo(DS, Declarator::ConditionContext);
   ParseDeclarator(DeclaratorInfo);
 
   // simple-asm-expr[opt]
   if (Tok.is(tok::kw_asm)) {
     SourceLocation Loc;

cfe/trunk/lib/Sema/SemaStmt.cpp

Sema::ActOnIfStmt(SourceLocation IfLoc, FullExprArg CondVal, Decl *CondVar,
   }
 
   ExprResult CondResult(CondVal.release());
 
   VarDecl *ConditionVar = nullptr;
   if (CondVar) {
     ConditionVar = cast<VarDecl>(CondVar);
     CondResult = CheckConditionVariable(ConditionVar, IfLoc, true);
+    CondResult = ActOnFinishFullExpr(CondResult.get(), IfLoc);
     if (CondResult.isInvalid())
       return StmtError();
   }
   Expr *ConditionExpr = CondResult.getAs<Expr>();
   if (!ConditionExpr)
     return StmtError();
 
   DiagnoseUnusedExprResult(thenStmt);
Sema::ActOnStartOfSwitchStmt(SourceLocation SwitchLoc, Expr *Cond,
   if (CondResult.isInvalid()) return StmtError();
   Cond = CondResult.get();
 
   // C99 6.8.4.2p5 - Integer promotions are performed on the controlling expr.
   CondResult = UsualUnaryConversions(Cond);
   if (CondResult.isInvalid()) return StmtError();
   Cond = CondResult.get();
 
-  if (!CondVar) {
-    CondResult = ActOnFinishFullExpr(Cond, SwitchLoc);
-    if (CondResult.isInvalid())
-      return StmtError();
-    Cond = CondResult.get();
-  }
 
   getCurFunction()->setHasBranchIntoScope();
 
   SwitchStmt *SS = new (Context) SwitchStmt(Context, ConditionVar, Cond);
   getCurFunction()->SwitchStack.push_back(SS);
   return SS;
 }
 

 Sema::ActOnWhileStmt(SourceLocation WhileLoc, FullExprArg Cond,
                      Decl *CondVar, Stmt *Body) {
   ExprResult CondResult(Cond.release());
 
   VarDecl *ConditionVar = nullptr;
   if (CondVar) {
     ConditionVar = cast<VarDecl>(CondVar);
     CondResult = CheckConditionVariable(ConditionVar, WhileLoc, true);
+    CondResult = ActOnFinishFullExpr(CondResult.get(), WhileLoc);
     if (CondResult.isInvalid())
       return StmtError();
   }
   Expr *ConditionExpr = CondResult.get();
   if (!ConditionExpr)
     return StmtError();
   CheckBreakContinueBinding(ConditionExpr);
 
Sema::ActOnForStmt(SourceLocation ForLoc, SourceLocation LParenLoc,
   CheckForLoopConditionalStatement(*this, second.get(), third.get(), Body);
   CheckForRedundantIteration(*this, third.get(), Body);
 
   ExprResult SecondResult(second.release());
   VarDecl *ConditionVar = nullptr;
   if (secondVar) {
     ConditionVar = cast<VarDecl>(secondVar);
     SecondResult = CheckConditionVariable(ConditionVar, ForLoc, true);
+    SecondResult = ActOnFinishFullExpr(SecondResult.get(), ForLoc);
     if (SecondResult.isInvalid())
       return StmtError();
   }
 
   Expr *Third  = third.release().getAs<Expr>();
 
   DiagnoseUnusedExprResult(First);
   DiagnoseUnusedExprResult(Third);

cfe/trunk/test/CXX/drs/dr9xx.cpp

#if __cplusplus >= 201103L
   struct D {
     D();
     D(std::initializer_list<int>);
     D(std::initializer_list<double>);
   };
   D d{};
 #endif
 }
+
+namespace dr948 { // dr948: 3.7
+#if __cplusplus >= 201103L
+  class A {
+  public:
+     constexpr A(int v) : v(v) { }
+     constexpr operator int() const { return v; }
+  private:
+     int v;
+  };
+
+  constexpr int id(int x)
+  {
+    return x;
+  }
+
+  void f() {
+     if (constexpr int i = id(101)) { }
+     switch (constexpr int i = id(2)) { default: break; case 2: break; }
+     for (; constexpr int i = id(0); ) { }
+     while (constexpr int i = id(0)) { }
+
+     if (constexpr A i = 101) { }
+     switch (constexpr A i = 2) { default: break; case 2: break; }
+     for (; constexpr A i = 0; ) { }
+     while (constexpr A i = 0) { }
+  }
+#endif
+}