Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
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 | ||
---|---|---|
1035 | Nice catch :) | |
clang/lib/Sema/SemaExprCXX.cpp | ||
1482 | Please use an explicit type here. | |
1483–1487 | 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 | ||
---|---|---|
1035 | 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)? | |
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? | |
26 | I'd also check auto{1,2} (ill-formed, does not deduce initializer_list<int>) and auto{{1,2}} (also ill-formed). |
clang/lib/Parse/ParseDeclCXX.cpp | ||
---|---|---|
1035 | 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)) { ? |
clang/lib/Parse/ParseDeclCXX.cpp | ||
---|---|---|
1035 | The r0 patch makes decltype(auto *) diagnose in the same way decltype(long *) do; looks good enough to me. | |
clang/lib/Sema/SemaExprCXX.cpp | ||
1483–1487 | 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 :/ |
Address review comments
- more test cases
- render auto({}) and new auto({}) invalid
- disambiguate auto(x)->n that looks like a declaration
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.
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. |
- revert the tentative parsing fix
- warn in -Wpre-c++2b-compat and -Wdecltype-auto-cast (ExtWarn)
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. |
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?
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...
clang/include/clang/Basic/DiagnosticSemaKinds.td | ||
---|---|---|
2391 | 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 | ||
1035 | ||
clang/test/CXX/dcl.dcl/dcl.spec/dcl.type/dcl.type.auto.deduct/p2.cpp | ||
5 |
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. |
clang/include/clang/Basic/DiagnosticSemaKinds.td | ||
---|---|---|
2391 | 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. |
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 | ||
---|---|---|
2391 | Thanks! I think ExtWarn is fine based on that. |
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++.
- 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).
(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.
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.
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 | ||
---|---|---|
60 |
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 | ||
---|---|---|
2327 | Mild preference to merge at least 2 of these 3 diagnostics. |
Mild preference to merge at least 2 of these 3 diagnostics.