This is an archive of the discontinued LLVM Phabricator instance.

[c++2b] Implement P0849R8 auto(x)
ClosedPublic

Authored by lichray on Nov 8 2021, 4:10 AM.

Diff Detail

Event Timeline

lichray requested review of this revision.Nov 8 2021, 4:10 AM
lichray created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptNov 8 2021, 4:10 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
lichray updated this revision to Diff 385450.Nov 8 2021, 4:20 AM

Document updates

lichray updated this revision to Diff 385454.Nov 8 2021, 4:25 AM

C++2y -> C++2b

lichray updated this revision to Diff 385564.Nov 8 2021, 10:47 AM

Clang-format

Thanks, this generally looks great.

It looks like we'll need some additional work on disambiguation to handle cases like:

struct A { int n; } a;
void f() { auto(&a)->n = 0; }

I think that's valid, but right now we misparse it as a declaration of a variable &a. (This syntax also resembles a function declaration with a trailing return type, but that interpretation should be rejected for various reasons, such as because n isn't a type.) This disambiguation failure seems to be specific to auto; this similar case is parsed properly:

struct A { int n; } a;
using T = A*;
void f() { T(&a)->n = 1; }
clang/lib/Parse/ParseDeclCXX.cpp
1032

Nice catch :)

clang/lib/Sema/SemaExprCXX.cpp
1481

Please use an explicit type here.

1482–1486

You should only do this if ListInitialization is true. Otherwise we'll accept the invalid syntax auto({a}).

clang/test/CXX/expr/expr.post/expr.type.conv/p1-2b.cpp
20

We're missing a test that we reject auto({a}). I assume that's what this line was intended to do.

I also wonder whether we should accept decltype(auto)(x) as an extension, but we can discuss that separately.

Comments on tests.

clang/lib/Parse/ParseDeclCXX.cpp
1032

I see zero hits for git grep 'decltype[(]auto[^)] clang/. So it seems this corner of the grammar has been missing any tests for a long time, but I hope this PR will add some.

decltype(auto*) i = 42; // should be a syntax error
decltype(auto(42)) i = 42;  // should be OK 
decltype(auto()) i = 42;  // should be a syntax error

Right now I don't see any tests for this stuff in this PR either. So it needs some.

clang/test/CXX/dcl.dcl/dcl.spec/dcl.type/dcl.type.auto.deduct/p2.cpp
7–29

It is definitely a good idea to repeat all of these tests with braces too: auto{v}, auto{"lit"}, etc.

52

Should you check that __is_same(decltype(auto(*this)), A)?
Should you check that __is_same(decltype(auto(this)), A*)?
Should you test inside a const member function too?
Should you check that the friend function actually can call auto(some_a_object), the same way it can call A(some_a_object) — and more to the point, that a non-friend can't?

57

I'm not sure which test file it should go in, but you definitely want a test somewhere proving that auto doesn't copy prvalues. E.g.

struct Uncopyable {
    explicit Uncopyable(int);
    Uncopyable(Uncopyable&&) = delete;
};
Uncopyable u = auto(Uncopyable(auto(Uncopyable(42))));
clang/test/CXX/expr/expr.post/expr.type.conv/p1-2b.cpp
18–19

What is the significance of the whitespace here? Is this a lexer test?
Normally it'd be just auto(a), auto{a} — just like T(a), T{a}. I think we should follow convention (again, unless this is a lexer test — in which case we should probably test both syntaxes with and without whitespace).

26

I'd also check auto{1,2} (ill-formed, does not deduce initializer_list<int>) and auto{{1,2}} (also ill-formed).

rsmith added inline comments.Nov 12 2021, 6:04 PM
clang/lib/Parse/ParseDeclCXX.cpp
1032

Some of this is tested in the new file test/CXX/dcl.dcl/dcl.spec/dcl.type/dcl.type.auto.deduct/p2.cpp (you might need to expand it manually; full-page search for decltype(auto( might otherwise not find it). It looks like we are missing tests for things like decltype(auto*). It's not obvious to me what diagnostic would be most useful if decltype(auto is not followed by ), (, or {, but my guess is that "expected )" pointing at the *, rather than an error on the auto token, would be the least surprising. So maybe this should be

if (Tok.is(tok::kw_auto) && NextToken().isNot(tok::l_paren, tok::l_brace)) {

?

lichray marked 10 inline comments as done.Nov 13 2021, 5:30 AM
lichray added inline comments.
clang/lib/Parse/ParseDeclCXX.cpp
1032

The r0 patch makes decltype(auto *) diagnose in the same way decltype(long *) do; looks good enough to me.

clang/lib/Sema/SemaExprCXX.cpp
1482–1486

My bad. Deemed new auto({a}) as valid; now corrected.

clang/test/CXX/dcl.dcl/dcl.spec/dcl.type/dcl.type.auto.deduct/p2.cpp
52

All good ideas.

clang/test/CXX/expr/expr.post/expr.type.conv/p1-2b.cpp
18–19

Blame clang-format :/

lichray updated this revision to Diff 387016.Nov 13 2021, 5:34 AM
lichray marked 3 inline comments as done.

Address review comments

  • more test cases
  • render auto({}) and new auto({}) invalid
  • disambiguate auto(x)->n that looks like a declaration
lichray updated this revision to Diff 387017.Nov 13 2021, 6:04 AM
  • Manually adjust auto{x} formatting
lichray marked an inline comment as done.Nov 13 2021, 6:07 AM
lichray updated this revision to Diff 389304.Nov 23 2021, 1:22 PM
  • Implement decltype(auto)(x) as well
lichray edited the summary of this revision. (Show Details)Nov 23 2021, 1:24 PM
lichray edited the summary of this revision. (Show Details)

It looks like we'll need some additional work on disambiguation to handle cases like:

struct A { int n; } a;
void f() { auto(&a)->n = 0; }

I think that's valid, but right now we misparse it as a declaration of a variable &a. [...]:

struct A { int n; } a;
using T = A*;
void f() { T(&a)->n = 1; }

Solved the issue and added test cases.

I also wonder whether we should accept decltype(auto)(x) as an extension, but we can discuss that separately.

Implemented. To produce a reference rather than a temporary, I had to special-case it, but in the end, it costs fewer lines of diff.

lichray updated this revision to Diff 389890.Nov 25 2021, 5:06 PM

Ping. Please review.

  • Reformat code after clang-format lands D114519

Generally this looks good. In addition to the ExtWarn for the decltype(auto) case, I think we're also missing a -Wc++20-compat warning for the feature as a whole.

clang/test/CXX/dcl.dcl/dcl.spec/dcl.type/dcl.type.auto.deduct/p2.cpp
5

We should produce an ExtWarn for this case.

lichray updated this revision to Diff 390946.Dec 1 2021, 1:05 AM
  • revert the tentative parsing fix
  • warn in -Wpre-c++2b-compat and -Wdecltype-auto-cast (ExtWarn)
lichray marked an inline comment as done.Dec 1 2021, 1:22 AM

I reverted the attempt to fix auto(&a)->n = 0; in this revision. The fix is definitely not 3 lines and does not fit in this patch.
Long story short, our tentative parsing code does not look at trailing return types and fails to disambiguate template <class T> void e(auto (*p)(T y) -> decltype(y())) {} if it is still ambiguous after seen auto (. The following files have more interesting cases:

CXX/dcl.dcl/dcl.spec/dcl.type/dcl.spec.auto/p2.cpp
CXX/dcl/dcl.fct/p17.cpp
CodeGenCXX/mangle-exprs.cpp
Parser/cxx0x-ambig.cpp
Parser/cxx1z-decomposition.cpp
SemaCXX/trailing-return-0x.cpp
SemaTemplate/alias-templates.cpp

and not all of them are correct. It needs a closer look in a future issue.

clang/test/CXX/dcl.dcl/dcl.spec/dcl.type/dcl.type.auto.deduct/p2.cpp
5

Fixed. Now silenced in this file and produced somewhere else.

lichray marked an inline comment as done.Dec 1 2021, 1:23 AM

Am I seeing correctly that there's no feature-test macro for auto(x)?

I'm proposing to introduce #define _LIBCPP_AUTO_CAST(expr) into libc++, so that we can use it for Ranges things that specify in terms of auto(x)... but if I want to make it conditionally expand into auto(x) on compilers that support the feature, I basically have to check the compiler vendor and version number? No feature-test macro?

Am I seeing correctly that there's no feature-test macro for auto(x)?

Oh, no, there isn't, and there wasn't a __cpp_auto either. I'll open a core issue.

I basically have to check the compiler vendor and version number? No feature-test macro?

For now, yes...

lichray updated this revision to Diff 407427.Feb 10 2022, 1:58 AM
  • Retarget Clang 15
aaron.ballman added inline comments.Feb 10 2022, 5:53 AM
clang/include/clang/Basic/DiagnosticSemaKinds.td
2392

Is there a reason this one should be ExtWarn instead of Extension? (I think we typically only issue this kind of diagnostic when -pedantic is specified.)

clang/lib/Parse/ParseDeclCXX.cpp
1032
clang/test/CXX/dcl.dcl/dcl.spec/dcl.type/dcl.type.auto.deduct/p2.cpp
5

We should produce an ExtWarn for this case.

Can you help me understand our rule for when to use ExtWarn vs Extension for this? I was under the impression we used Extension for this sort of thing, and only used ExtWarn in the cases where the use of the extension is wrong and the warning should be on by default.

lichray updated this revision to Diff 408079.Feb 11 2022, 3:22 PM
  • Rephrase a warning message
lichray marked 2 inline comments as done.Feb 11 2022, 4:22 PM
lichray added inline comments.
clang/include/clang/Basic/DiagnosticSemaKinds.td
2392

I took a look at other warnings. It seems that Clang believes that a portable program can contain Extensions; if ExtWarn is issued, the program is not likely to be portable.

I asked GCC folks yesterday to see if they want to implement this extension, and they are not interested. So I guess ExtWarn is suitable for now.

lichray marked an inline comment as done.Feb 11 2022, 7:49 PM

I spotted some test coverage that I think we should add:

static_cast<auto>(whatever);
reinterpret_cast<auto>(whatever);
const_cast<auto>(whatever);
dynamic_cast<auto>(whatever);
(auto)whatever;

I believe all of those should be rejected because of https://eel.is/c++draft/dcl.spec.auto#general-5.sentence-2.

Are there changes needed for the AST printer for this new form of cast notation?

I suppose one other question worth asking: if we're allowing decltype(auto)(whatever) as a Clang extension, should we be accepting __typeof__(auto)(whatever) as well?

clang/include/clang/Basic/DiagnosticSemaKinds.td
2392

Thanks! I think ExtWarn is fine based on that.

lichray updated this revision to Diff 410915.Feb 23 2022, 12:47 PM
  • Add more tests to diagnose other forms of casts
lichray marked an inline comment as done.Feb 23 2022, 1:21 PM

I spotted some test coverage that I think we should add:

static_cast<auto>(whatever);
reinterpret_cast<auto>(whatever);
const_cast<auto>(whatever);
dynamic_cast<auto>(whatever);
(auto)whatever;

Done.

Are there changes needed for the AST printer for this new form of cast notation?

Wow, I broke AST print. CTAD works in expressions and new because their specializations are valid type-ids...

I suppose one other question worth asking: if we're allowing decltype(auto)(whatever) as a Clang extension, should we be accepting __typeof__(auto)(whatever) as well?

We don't allow __typeof__(auto) type-specifier at the moment, so not yet. I think it may be more reasonable to allow both __typeof__(auto) and typeof(auto) when implementing C23 typeof. Their expression forms should be useful in C++.

lichray updated this revision to Diff 411264.Feb 24 2022, 4:38 PM
  • Do not overwrite braced inits when forming AST
  • Explain why losing decltype(auto) as written in AST

Also implemented decltype(auto)(x) (https://wg21.link/p0849r2) as a Clang extension.

I'd like to better understand the use cases for this. WG21 considered this as part of the feature and ultimately rejected it (fairly strongly, according to the EWG polls). From the meeting minutes, it says that the reason EWG disliked the idea is because of its expert-friendly nature and that this facility would present teachability issues. I tend to agree, so I'm wondering why we want to add the feature as an extension when WG21 explicitly rejected it (that doesn't seem in line with our extension policy: https://clang.llvm.org/get_involved.html#criteria).

Also implemented decltype(auto)(x) (https://wg21.link/p0849r2) as a Clang extension.

I'd like to better understand the use cases for this. WG21 considered this as part of the feature and ultimately rejected it (fairly strongly, according to the EWG polls). From the meeting minutes, it says that the reason EWG disliked the idea is because of its expert-friendly nature and that this facility would present teachability issues. I tend to agree, so I'm wondering why we want to add the feature as an extension when WG21 explicitly rejected it (that doesn't seem in line with our extension policy: https://clang.llvm.org/get_involved.html#criteria).

(I'm not directly answering Aaron's question.) Personally I think it makes sense to support decltype(auto)(x) in the same release as auto(x), for the same reason we supported Constrained decltype(auto) in the same release as Constrained auto. But, rather than have that essentially tangential policy discussion in this PR, @lichray, would you be willing to split out all the changes related to decltype(auto)(x) into their own separate PR, (1) so that we could land the uncontroversial C++20 auto(x) parts sooner rather than later, and (2) so that if three years from now someone does decide to revert decltype(auto)(x), all its changes will be nicely isolated in their own commit instead of mixed in with important bits of our C++20 conformance? And (3) so that we could have the "Should we do this?" policy discussion in that PR, instead of here, and (4) the discussion would be more focused because we could see exactly the set of diffs we're talking about, instead of mixed in etc etc.

Also implemented decltype(auto)(x) (https://wg21.link/p0849r2) as a Clang extension.

I'd like to better understand the use cases for this. WG21 considered this as part of the feature and ultimately rejected it (fairly strongly, according to the EWG polls). From the meeting minutes, it says that the reason EWG disliked the idea is because of its expert-friendly nature and that this facility would present teachability issues. I tend to agree, so I'm wondering why we want to add the feature as an extension when WG21 explicitly rejected it (that doesn't seem in line with our extension policy: https://clang.llvm.org/get_involved.html#criteria).

(I'm not directly answering Aaron's question.) Personally I think it makes sense to support decltype(auto)(x) in the same release as auto(x), for the same reason we supported Constrained decltype(auto) in the same release as Constrained auto. But, rather than have that essentially tangential policy discussion in this PR, @lichray, would you be willing to split out all the changes related to decltype(auto)(x) into their own separate PR, (1) so that we could land the uncontroversial C++20 auto(x) parts sooner rather than later, and (2) so that if three years from now someone does decide to revert decltype(auto)(x), all its changes will be nicely isolated in their own commit instead of mixed in with important bits of our C++20 conformance? And (3) so that we could have the "Should we do this?" policy discussion in that PR, instead of here, and (4) the discussion would be more focused because we could see exactly the set of diffs we're talking about, instead of mixed in etc etc.

This option sounds quite sensible to me!

This option sounds quite sensible to me!

I'd like that too!

lichray updated this revision to Diff 411493.Feb 25 2022, 12:21 PM
  • Remove decltype(auto) from the patch
  • Parenthesize deduced casts in AST print
lichray edited the summary of this revision. (Show Details)Feb 25 2022, 12:21 PM
lichray added a comment.EditedFeb 25 2022, 12:33 PM

Are there changes needed for the AST printer for this new form of cast notation?

Fixed one pre-existing issue, and now emit legal C++ code, except in one corner case. But CXXUnresolvedConstructExpr is a cause of more problems than that, so not touching it for now. Fixed the remaining issues in D120608.

lichray updated this revision to Diff 411510.EditedFeb 25 2022, 1:22 PM
lichray edited the summary of this revision. (Show Details)

Rebase

lichray updated this revision to Diff 411571.Feb 25 2022, 8:55 PM

Re-run build

aaron.ballman accepted this revision.Feb 28 2022, 10:20 AM

The changes here LGTM, but I'd appreciate a second set of eyes on it in case I missed anything before this lands.

clang/test/CXX/dcl.dcl/dcl.spec/dcl.type/dcl.spec.auto/p5.cpp
65
This revision is now accepted and ready to land.Feb 28 2022, 10:20 AM
cor3ntin resigned from this revision.Feb 28 2022, 10:21 AM
erichkeane accepted this revision.Feb 28 2022, 10:27 AM

Just did a pass through, nothing suspicious, I think this looks good.

Mild preference to merge 2 or 3 diagnostics based on how similar they all are, but that can be done without further review, or as a separate commit (or not at all, if no one else cares).

clang/include/clang/Basic/DiagnosticSemaKinds.td
2331

Mild preference to merge at least 2 of these 3 diagnostics.

lichray updated this revision to Diff 411839.Feb 28 2022, 10:33 AM

Use the right copy of clang-format

lichray marked an inline comment as done.Feb 28 2022, 10:33 AM
This revision was landed with ongoing or failed builds.Feb 28 2022, 5:21 PM
This revision was automatically updated to reflect the committed changes.