Page MenuHomePhabricator

[Parse] Allow 'constexpr' in condition declarations

Authored by meadori on Apr 10 2015, 10:19 AM.



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.

Diff Detail


Event Timeline

meadori updated this revision to Diff 23609.Apr 10 2015, 10:19 AM
meadori retitled this revision from to [Parse] Allow 'constexpr' in condition declarations.
meadori updated this object.
meadori edited the test plan for this revision. (Show Details)
meadori added a reviewer: rsmith.
meadori added a subscriber: Unknown Object (MLST).
rsmith edited edge metadata.Apr 18 2015, 6:19 PM


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?

1760 ↗(On Diff #23609)

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

2585 ↗(On Diff #23609)

Do your tests cover the need for this?

4–5 ↗(On Diff #23609)

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

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

class T {
   constexpr T(int v) : v(v) { }
   constexpr operator int() const { return v; }
   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.

1760 ↗(On Diff #23609)

Will fix.

2585 ↗(On Diff #23609)

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

4–5 ↗(On Diff #23609)

Will fix.

meadori updated this revision to Diff 24914.May 4 2015, 2:08 PM
meadori edited edge metadata.

Address review feedback.

rsmith added a comment.May 4 2015, 6:14 PM

This is looking good.

1689 ↗(On Diff #24914)

I think this should be called DSC_condition?

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 added inline comments.May 5 2015, 10:13 AM
1689 ↗(On Diff #24914)

Agreed. Will fix.

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 updated this revision to Diff 27520.EditedJun 11 2015, 9:27 AM

Here is a new version with the previous feedback incorporated:

  1. Use 'DSC_condition' instead of 'DSC_constexpr'.
  1. 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.

It looks like you uploaded the wrong patch.

meadori updated this revision to Diff 27546.Jun 11 2015, 2:38 PM

Uploaded the wrong patch before.

rsmith accepted this revision.Jun 25 2015, 2:00 PM
rsmith edited edge metadata.
rsmith added inline comments.
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.

This revision is now accepted and ready to land.Jun 25 2015, 2:00 PM
This revision was automatically updated to reflect the committed changes.