[PATCH] D68845: Don't emit unwanted constructor calls in co_return statements

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

Quuxplusone added inline comments.


================
Comment at: clang/lib/Sema/SemaCoroutine.cpp:896
+      if (S.CanPerformCopyInitialization(Entity, &AsRvalue))
+        return true;
+    } else if (auto *FTD = dyn_cast<FunctionTemplateDecl>(D)) {
----------------
aaronpuchert wrote:
> Overlad resolution can actually still fail if there is a viable candidate, namely when there are multiple candidates and none is better than all others. It's a bit weird though to fall back to lvalue parameter then as if nothing happened.
That is an interesting point! I had not considered it during [P1155](https://wg21.link/p1155r2). I imagine that it might make implementation of [P1155](https://wg21.link/p1155r2)'s new logic more difficult.

GCC 8 (but not trunk) implements the behavior I would expect to see for derived-to-base conversions: https://godbolt.org/z/fj_lNw

C++ always treats "an embarrassment of riches" equivalently to a "famine"; overload resolution //can// fail due to ambiguity just as easily as it can fail due to no candidates at all. I agree it's "a bit weird," but it would be vastly weirder if C++ did anything //different// from its usual behavior in this case.

I'm still amenable to the idea that `co_return` should simply not do the copy-elision or implicit-move optimizations at all. I wish I knew some use-cases for `co_return`, so that we could see if the optimization is useful to anyone.


================
Comment at: clang/test/SemaCXX/coroutine-rvo.cpp:65
+  NoCopyNoMove value;
+  co_return value;
+}
----------------
@aaronpuchert wrote:
> Given the complexities of this implementation, I'm beginning to doubt whether implicit moves make sense for `co_return` at all. Since there can never be any kind of RVO, why not always require an explicit `std::move`? Implicit moves on return were introduced because an explicit move would inhibit NRVO, and without move there wouldn't be a graceful fallback for implementations that don't have NRVO.

I still think that it makes sense to do implicit-move on //any// control-flow construct that "exits" the current function. Right now that's `return` and `throw`, and they both do implicit-move (albeit inconsistently with plenty of implementation divergence documented in [P1155](http://wg21.link/p1155r2)). C++2a adds `co_return` as another way to "exit." I think it would be un-graceful and inconsistent to permit `return p` but require `co_return std::move(p)`.

Now, I do think that C++2a Coroutines are needlessly complicated, which makes the implementation needlessly complicated. You've noticed that to codegen `t.return_value(x)` requires not just overload resolution, but actually figuring out whether `task::return_value` is a function at all — it might be a static data member of function-pointer type, or something [even wilder](https://quuxplusone.github.io/blog/2019/09/18/cppcon-whiteboard-puzzle/). But eliminating implicit-move won't make that complexity go away, will it? Doing implicit-move just means doing the required overload resolution twice instead of once. That //should// be easy, compared to the rest of the mess.

That said, I would be happy to start a thread among you, me, David Stone, and the EWG mailing list to consider removing the words "or `co_return`" from [class.copy.elision/3](http://eel.is/c++draft/class.copy.elision#3.1). Say the word.

[EDIT: aw shoot, I never submitted this comment last time]


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68845