[PATCH] D119184: [clang] [concepts] Check constrained-auto return types for void-returning functions

[Arthur O'Dwyer via Phabricator via cfe-commits]

Quuxplusone marked an inline comment as not done.
Quuxplusone added inline comments.


================
Comment at: clang/lib/Sema/SemaStmt.cpp:3816
+        Context.getTrivialTypeSourceInfo(Context.VoidTy, ReturnLoc), ReturnLoc);
+    Expr *Dummy = R.get();
+    DeduceAutoResult DAR = DeduceAutoType(OrigResultType, Dummy, Deduced);
----------------
sammccall wrote:
> why initialize into an ExprResult instead of an Expr* directly?
Because I didn't know the latter was possible. :) Btw, I assume there's no worry here about how the Expr will ultimately get garbage-collected, right? All the code I looked at seems pretty cavalier about it, so I assume `new (Context)` magically takes care of the memory management part.


================
Comment at: clang/lib/Sema/SemaStmt.cpp:3819
+    if (DAR == DAR_Failed && !FD->isInvalidDecl())
+      Diag(ReturnLoc, HasReturnStmt ? diag::err_auto_fn_return_void_but_not_auto
+                                    : diag::err_auto_fn_no_return_but_not_auto)
----------------
sammccall wrote:
> (BTW, err_auto_fn_return_void_but_not_auto has no testcases, feel free to add one near `deduced-return-type-cxx14.cpp:341`, or not)
Will do.


================
Comment at: clang/test/SemaCXX/deduced-return-type-cxx14.cpp:116
 
-auto &void_ret_2() {} // expected-error {{cannot deduce return type 'auto &' for function with no return statements}}
+auto &void_ret_2() {} // expected-error {{cannot form a reference to 'void'}}
 const auto void_ret_3() {} // ok, return type 'const void' is adjusted to 'void'
----------------
sammccall wrote:
> This feels like a regression, this diagnostic is just attached to the end of the function and there's no longer any explicit indication that the return type or deduction is involved.
> 
> It may be worth keeping the explicit check in `ActOnFinishFunctionBody` before the deduction happens to improve the diagnostic in this case.
> 
> (I don't think it's important to do this in the return-with-no-argument case, since the error will point at the return statement which provides enough context I think)
I agree about the (mild) regression but am not sure what to do about it. I think it might help if Clang emitted a note `note: during return type deduction here` (pointer to the offending `return` statement or end-of-function); do you think I should do that? and if so, how?

> It may be worth keeping the explicit check in `ActOnFinishFunctionBody` before the deduction happens to improve the diagnostic in this case.

I can't picture what you mean; can you suggest a specific patch?

In case it matters, I'm a little leery of hard-coding a special case like we had in the old `DeduceFunctionTypeFromReturnExpr` lines 3814–3816,
```
    // Deduction here can only succeed if the return type is exactly 'cv auto'
    // or 'decltype(auto)', so just check for that case directly.
    if (!OrigResultType.getType()->getAs<AutoType>()) {
```
because special-casing `void` is kind of how we got into this mess in the first place.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D119184/new/

https://reviews.llvm.org/D119184