Page MenuHomePhabricator

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

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

Details

Summary

https://wg21.link/p0849

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

Diff Detail

Event Timeline

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

Document updates

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

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

lichray updated this revision to Diff 385564.Mon, Nov 8, 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.Fri, Nov 12, 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.Sat, Nov 13, 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.Sat, Nov 13, 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.Sat, Nov 13, 6:04 AM
  • Manually adjust auto{x} formatting
lichray marked an inline comment as done.Sat, Nov 13, 6:07 AM
lichray updated this revision to Diff 389304.Tue, Nov 23, 1:22 PM
  • Implement decltype(auto)(x) as well
lichray edited the summary of this revision. (Show Details)Tue, Nov 23, 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.Thu, Nov 25, 5:06 PM

Ping. Please review.

  • Reformat code after clang-format lands D114519