This is an archive of the discontinued LLVM Phabricator instance.

Allow conditions to be decomposed with structured bindings
ClosedPublic

Authored by lichray on Oct 25 2017, 4:35 AM.

Details

Summary

This feature was discussed but not yet proposed. It allows a structured binding to appear as a condition

if (auto [ok, val] = f(...))

So the user can save an extra condition if the statement can test the value to-be-decomposed instead. Formally, it makes the value of the underlying object of the structured binding declaration also the value of a condition that is an initialized declaration.

Considering its logicality which is entirely evident from its trivial implementation, I think it might be acceptable to land it as an extension for now before I write the paper.

Event Timeline

lichray created this revision.Oct 25 2017, 4:35 AM

I'm not opposed to the functionality, but this isn't a part of C++2a either; I think we should be diagnosing this code with a warning so users don't expect it to work on every compiler.

I'm not opposed to the functionality, but this isn't a part of C++2a either; I think we should be diagnosing this code with a warning so users don't expect it to work on every compiler.

C++2a the standard itself is under development, so the users can't expect any feature to work on every compiler already.

lichray edited the summary of this revision. (Show Details)Oct 25 2017, 10:24 AM
lichray removed a reviewer: cfe-commits.

I'm not opposed to the functionality, but this isn't a part of C++2a either; I think we should be diagnosing this code with a warning so users don't expect it to work on every compiler.

C++2a the standard itself is under development, so the users can't expect any feature to work on every compiler already.

I'm aware, but I was unaware that we've accepted this functionality in C++2a yet within WG21. Did we vote this in and I simply didn't remember it?

I'm aware, but I was unaware that we've accepted this functionality in C++2a yet within WG21. Did we vote this in and I simply didn't remember it?

No. In the first line of the Summary I said this hasn't even been proposed. But on the reflectors it received positive feedback. While I was trying to implement it in order to find out corner cases which I haven't thought of, I found that the implementation is astonishingly simple, defending the change by showing that not introducing the change to the language is merely complicating the language, so I decided to seeking for a possibility to land it first.

I'm aware, but I was unaware that we've accepted this functionality in C++2a yet within WG21. Did we vote this in and I simply didn't remember it?

No. In the first line of the Summary I said this hasn't even been proposed. But on the reflectors it received positive feedback. While I was trying to implement it in order to find out corner cases which I haven't thought of, I found that the implementation is astonishingly simple, defending the change by showing that not introducing the change to the language is merely complicating the language, so I decided to seeking for a possibility to land it first.

Okay, then I didn't misunderstand you and we're on the same page.

We typically diagnose vendor extensions to the language, and I think we should apply that consistently. Otherwise, your code will compile fine in Clang with warning levels cranked all the way up and then fail to compile on every other compiler, which does not do our users any good. I'll let Richard have the final say for this, but my preference is that this is diagnosed as an extension (at least in pedantic mode).

lichray updated this revision to Diff 120295.Oct 25 2017, 12:35 PM

One more test case

We typically diagnose vendor extensions to the language, and I think we should apply that consistently. Otherwise, your code will compile fine in Clang with warning levels cranked all the way up and then fail to compile on every other compiler, which does not do our users any good. I'll let Richard have the final say for this, but my preference is that this is diagnosed as an extension (at least in pedantic mode).

I think we don't have a practice to issue diagnosis for a change that is basically racing with the standard. First, if we issue a diagnosis now, there is a chance for the (coming) proposal to be accepted before we making a release from trunk; second, users are supposed to be cautious using latest -std= in general, as there might be coming fixes. In addition, this change, most of the time, requires the users to opt-in by providing conversion operators, so users are expected to be intentionally trying it out rather than accidentally running into a situation that you describe.

But anyway, ping @rsmith?

rsmith added inline comments.Oct 30 2017, 11:24 AM
include/clang/Sema/DeclSpec.h
2015

If there's no grammar ambiguities here (and I don't think there are), please accept this unconditionally, with an ExtWarn.

lib/Parse/ParseExprCXX.cpp
1711

This shouldn't be listed as C++2a until it's actually voted into the working draft. Listing it as [Clang] for a clang extension is fine.

lib/Sema/SemaDeclCXX.cpp
696–698

Again, please don't guess what's going to be in C++2a. I would actually expect that we'll vote this in as a DR against C++17. It seems like a wording oversight to me.

lichray updated this revision to Diff 120913.Oct 30 2017, 4:34 PM

Make the feature unconditional with an ExtWarn

lichray retitled this revision from [c++2a] Decomposed _condition_ to Allow conditions to be decomposed with structured bindings.Oct 30 2017, 4:37 PM
lichray marked 3 inline comments as done.Oct 30 2017, 4:45 PM
lichray added inline comments.
lib/Sema/SemaDeclCXX.cpp
696–698

I would say, use DR with parsimony... But OK.

OT: I'm writing a paper on auto(x) and auto{x} while implementing it. Do you expect this to land as a DR? In which form you expect it to appear in Clang?

rsmith added inline comments.Oct 30 2017, 5:09 PM
lib/Sema/SemaDeclCXX.cpp
696–698

For that one, I don't know. The status quo is inconsistent (particularly comparing auto to class template argument deduction), but I don't think the inconsistency is a wording oversight. If this were to land in Clang prior to being voted into C++2a, I'd expect it to produce an ExtWarn that's not tied to any particular C++ version.

lichray updated this revision to Diff 120922.Oct 30 2017, 6:41 PM
lichray marked an inline comment as done.

Tweak coding style

rsmith added inline comments.Nov 30 2017, 5:27 PM
include/clang/Basic/DiagnosticSemaKinds.td
414 ↗(On Diff #120922)

Phrase this as "ISO C++17 does not permit structured binding declaration in a condition"

lib/Sema/SemaDeclCXX.cpp
715–716

Using a lambda here seems like unwarranted complexity. A three-way ternary of the form

!getLangOpts().CPlusPlus1z ? diag::ext_decomp_decl :
D.getContext() == Declarator::ConditionContext ? diag::ext_decomp_decl_cond :
diag::warn_cxx14_compat_decomp_decl

would be fine. Feel free to ignore clang-format if it wants to format it stupidly.

test/Misc/warning-flags.c
19 ↗(On Diff #120922)

Please read and respect this rule :)

test/Parser/cxx1z-decomposition.cpp
35–36 ↗(On Diff #120922)

You have removed test coverage here, at least for the if init-statement case. Please just update the comments on these tests rather than removing them.

lichray updated this revision to Diff 125288.Dec 3 2017, 4:12 AM
lichray marked 3 inline comments as done.

Rephrase warning message.

lichray added inline comments.Dec 3 2017, 4:12 AM
lib/Sema/SemaDeclCXX.cpp
715–716

The clang-formatted code actually looks good.

test/Misc/warning-flags.c
19 ↗(On Diff #120922)

Do you know of some categories which can cover this kind of extensions?

rsmith added inline comments.Dec 5 2017, 11:42 AM
test/Misc/warning-flags.c
19 ↗(On Diff #120922)

Add a new warning flag for it. Maybe -Wbinding-in-condition?

lichray marked an inline comment as done.Dec 5 2017, 6:58 PM
lichray added inline comments.
test/Misc/warning-flags.c
19 ↗(On Diff #120922)

I wish it's still alive after Clang 6.0 released :)

lichray updated this revision to Diff 125665.Dec 5 2017, 6:59 PM
lichray marked an inline comment as done.

Added -Wbinding-in-condition.

rsmith accepted this revision.Dec 5 2017, 7:10 PM

Thanks, LGTM!

This revision is now accepted and ready to land.Dec 5 2017, 7:10 PM

Can someone commit this please? One more patch then I'll go get a commit bit.

This revision was automatically updated to reflect the committed changes.