This Implements P2266 Simpler implicit move.
Signed-off-by: Matheus Izvekov <mizvekov@gmail.com>
Details
Details
Diff Detail
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
There are a very large number of changes, so older changes are hidden. Show Older Changes
| clang/lib/Sema/SemaTemplateInstantiateDecl.cpp | ||
|---|---|---|
| 1059 ↗ | (On Diff #333745) | I've always found it annoying that we only store the block type on the BlockExpr and not the BlockDecl, so if you've got a good reason to change that, I'm not opposed. That said, while I'm not an expert in how we do NRVO variable tracking, the code must have some ability to handle an unknown return type when just processing declarations by delaying some of the bookkeeping/analysis until the end of the function. It's possible but quite uncommon to declare a return type explicitly in a block, so that's the dominant case here. I'm actually surprised we do any of this tracking when we're just instantiating a variable. |
| clang/lib/Sema/SemaTemplateInstantiateDecl.cpp | ||
|---|---|---|
| 1059 ↗ | (On Diff #333745) | Thanks, your observations have been extremely helpful. So this whole NRVO tracking thing looks to be in bad shape. This tracker here fails to handle 'auto' types. I don't think this gets fixed up anywhere else. This is also badly covered by the test suite. The test included with the merge request that introduced this tracking (CodeGenCXX/nrvo.cpp) still passes if you either delete this whole block here, or if you just do Var->setNRVOVariable(D->isNRVOVariable()). In fact, almost the whole test suite passes with those changes, the only tests which fail are CodeGenCXX/matrix-type-builtins.cpp and CodeGenCXX/matrix-type-operators.cpp respectively. |
| clang/lib/Sema/SemaTemplateInstantiateDecl.cpp | ||
|---|---|---|
| 1059 ↗ | (On Diff #333745) | I'm surprised that this has any impact on matrix types; pinging @fhahn in case he understands what the interaction might be. It looks like we're assuming that the parse-time analysis of NRVO candidates is correct except for possibly needing adjustment for dependent expressions / return types. The parse-time analysis seems to work by keeping track of NRVO candidates on Scope; since we don't push meaningful Scopes during template instantiation, that won't work, so we have to trust the parse-time analysis. But the parse-time analysis can also be overly conservative, mostly because of if constexpr; that might be enough of a corner case to not merit something better, though. @rsmith is the expert here, though. At any rate, it should be *correct* to handle non-function cases as if the return type were dependent, the same way it presumably works for functions with deduced return types. |
| clang/lib/Sema/SemaTemplateInstantiateDecl.cpp | ||
|---|---|---|
| 1059 ↗ | (On Diff #333745) | The main thing here is that the NRVO criteria for function return type is ignored if the type is dependent. It is strange that we make this distinction between dependent and non-dependent auto here. This is actually the only case where this function is called where the FnRetType is possibly auto, so it's an understandable oversight if that is the case. I see we don't have the type deduced here even for functions with a single return statement and no if constexpr or other indirection, where it seems it would have been possible to have just deduced it to some dependent type during parsing. That said, my (pretty uninformed) guess here is that it should be safe to treat auto the same as dependent, and it is probably the most reasonable choice. |
Comment Actions
- test/SemaCXX/coroutine-rvo: Make MoveOnly trivial, since the temporary is not created anymore and there is no special reason to test a non-trivial type here.
Comment Actions
IMHO we should just land this already. (It's been sitting without major update since mid-April.) It is intended to affect only -std=c++2b mode, and implements a paper (full disclosure: my paper) targeting C++2b that EWG wants implementation experience with. So I have proposed to @mizvekov that he should land this on Monday, unless someone yells "stop" before then.
If this breaks anyone compiling in C++2b mode, then (1) it can always be reverted, and (2) that would be exactly the successful gathering of field experience that we're hoping for.
Comment Actions
Actually we can land it now. @rsmith gave me the go ahead in private. The windows pipeline passes. The latest debian build failure is some random fortran thing, so we are good.
Comment Actions
(In a build prior to https://reviews.llvm.org/rGc60dd3b2626a4d9eefd9f82f9a406b0d28d3fd72 "Revert '[clang] NRVO: Improvements and handling of more cases.'") I see the following (reduced from https://git.libreoffice.org/core/+/649313625b94e6b879848fc19b607b74375100bf/o3tl/qa/compile-temporary.cxx) started to fail under -std=c++2b with this change (and continues to compile fine with -std=c++20):
$ cat test.cc template <typename T> constexpr T& temporary(T&& x) { return x; } template <typename T> constexpr T& temporary(T&) = delete; void f(int*); int g(); void h() { f(&temporary(int())); f(&temporary(g())); } $ clang++ -std=c++2b -fsyntax-only test.cc test.cc:1:62: error: non-const lvalue reference to type 'int' cannot bind to a temporary of type 'int' template <typename T> constexpr T& temporary(T&& x) { return x; } ^ test.cc:7:8: note: in instantiation of function template specialization 'temporary<int>' requested here f(&temporary(int())); ^ test.cc:8:8: error: no matching function for call to 'temporary' f(&temporary(g())); ^~~~~~~~~ test.cc:2:36: note: candidate function [with T = int] not viable: expects an lvalue for 1st argument template <typename T> constexpr T& temporary(T&) = delete; ^ test.cc:1:36: note: candidate template ignored: substitution failure [with T = int] template <typename T> constexpr T& temporary(T&& x) { return x; } ^ 2 errors generated.
It is not clear to me whether that is an intended change in behavior according to http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2021/p2266r1.html "Simpler implicit move", or whether it is a bug in this implementation.
Comment Actions
Yeah that one is intentional, under P2266 as far as implicit move is concerned, that code behaves similarly to this code under C++20 (which is diagnosed for the same reason):
template <typename T> constexpr T& temporary(T&& x) { return static_cast<T&&>(x); }If you want this semantics with P2266, you could write:
template <typename T> constexpr T& temporary(T&& x) { return static_cast<T&>(x); }And the above also works on C++20.
Thank you for reporting this, it's the kind of feedback we needed about real world usage of code that would break under these new rules.
Comment Actions
@sberg: Thanks for the example! @mizvekov's comments are correct, but I wanted to put some more comments here for posterity. (I'll also try to find a place for this real-world example in the next revision of p2266.)
The code in question is o3tl::temporary, documented as "Cast an rvalue to an lvalue." and implemented as template<class T> T& temporary(T&& x) { return x; }
https://docs.libreoffice.org/o3tl/html/temporary_8hxx_source.html
p2266 proposes to break this code in C++23, by making the expression x an xvalue. EWG was well aware that p2266 would break this exact kind of "laundering" function, which takes an rvalue and launders it into an lvalue without a cast. This breakage was generally (though I'm sure not universally) seen as a good thing. We weren't aware of any specific real-world examples of such "laundering" functions, though, so this is very useful info.
As Matheus says, the best way to fix the code is to add an explicit cast: return static_cast<T&>(x);.
Comment Actions
For the record, the one other breakage of a LibreOffice build with this is the handful of places that needed https://git.libreoffice.org/core/+/433ab39b2175bdadb4916373cd2dc8e1aabc08a5%5E%21 "Adapt implicit OString return value construction to C++23 P2266R1": In a nutshell, LO has an OString class with (non-explicit) constructors from any kind of char pointer or array, where the "array of N const char" variant (no. 3 below) is special, as it determines the string length as N - 1 (whereas all the other variants search for the terminating NUL to determine the length). That requires another constructor overload (no. 2 below) for non-const arrays, taking a non-const lvalue reference, which now causes issues when that constructor shall implicitly be used in return statements:
$ cat test.cc #include <cstddef> template<typename> struct CharPtrDetector {}; template<> struct CharPtrDetector<char *> { using type = int; }; template<> struct CharPtrDetector<char const *> { using type = int; }; template<typename> struct NonConstCharArrayDetector {}; template<std::size_t N> struct NonConstCharArrayDetector<char [N]> { using type = int; }; template<typename> struct ConstCharArrayDetector {}; template<std::size_t N> struct ConstCharArrayDetector<char const [N]> { using type = int; }; struct OString { template<typename T> OString(T const &, typename CharPtrDetector<T>::type = 0); // #1 template<typename T> OString(T &, typename NonConstCharArrayDetector<T>::type = 0); // #2 template<typename T> OString(T &, typename ConstCharArrayDetector<T>::type = 0); // #3 }; OString good1a() { char * p; //... return p; // use #1 }; OString good1b() { char const * p; //... return p; // use #1 }; OString bad2() { char buf[10]; //... return buf; // use #2 } OString good3() { return "foo"; // use #3 } $ clang++ -std=c++20 -fsyntax-only test.cc $ clang++ -std=c++2b -fsyntax-only test.cc test.cc:41:12: error: no viable conversion from returned value of type 'char [10]' to function return type 'OString' return buf; // use #2 ^~~ test.cc:17:8: note: candidate constructor (the implicit copy constructor) not viable: no known conversion from 'char [10]' to 'const OString &' for 1st argument struct OString { ^ test.cc:17:8: note: candidate constructor (the implicit move constructor) not viable: no known conversion from 'char [10]' to 'OString &&' for 1st argument struct OString { ^ test.cc:21:5: note: candidate constructor [with T = char [10]] not viable: expects an lvalue for 1st argument OString(T &, typename NonConstCharArrayDetector<T>::type = 0); // #2 ^ test.cc:19:5: note: candidate template ignored: substitution failure [with T = char [10]]: no type named 'type' in 'CharPtrDetector<char [10]>' OString(T const &, typename CharPtrDetector<T>::type = 0); // #1 ^ ~~~~ test.cc:23:5: note: candidate template ignored: substitution failure [with T = char [10]]: no type named 'type' in 'ConstCharArrayDetector<char [10]>' OString(T &, typename ConstCharArrayDetector<T>::type = 0); // #3 ^ ~~~~ 1 error generated.
Comment Actions
- Change block capture NRVO semantics to match P2266 in C++2b mode.
- Update cxx_status.
Comment Actions
And, again for the record, with a build of LibreOffice with clang-cl (and -Xclang -std=c++2b) on Windows, at least against the C++ standard library from Visual Studio 2019 version 16.20.2, I ran into two issues in the standard library itself, when using std::getline and std::istream::operator>>:
In file included from C:/lo-clang/core/setup_native/source/win32/customactions/reg_dlls/reg_dlls.cxx:12:
C:/PROGRA~2/MIB055~1/2019/COMMUN~1/VC/Tools/MSVC/1429~1.300/Include\string(66,12): error: non-const lvalue reference to type 'basic_istream<...>' cannot bind to a temporary of type 'basic_istream<...>'
return _Istr;
^~~~~
C:/PROGRA~2/MIB055~1/2019/COMMUN~1/VC/Tools/MSVC/1429~1.300/Include\string(78,12): note: in instantiation of function template specialization 'std::getline<wchar_t, std::char_traits<wchar_t>, std::allocator<wchar_t>>' requested here
return getline(_STD move(_Istr), _Str, _Delim);
^
C:/lo-clang/core/setup_native/source/win32/customactions/reg_dlls/reg_dlls.cxx(197,17): note: in instantiation of function template specialization 'std::getline<wchar_t, std::char_traits<wchar_t>, std::allocator<wchar_t>>' requested here
while (std::getline(ss, sToken, L'|'))
^
1 error generated.
make[1]: *** [C:/lo-clang/core/solenv/gbuild/LinkTarget.mk:298: C:/lo-clang/core/workdir/CxxObject/setup_native/source/win32/customactions/reg_dlls/reg_dlls.o] Error 1
c:/Program Files (x86)/Microsoft Visual Studio/2019/Community/VC/Tools/MSVC/14.29.30037/include/stringand
[build CXX] l10ntools/source/merge.cxx
In file included from C:/lo-clang/core/l10ntools/source/merge.cxx:21:
In file included from C:/lo-clang/core/include\sal/log.hxx:20:
In file included from C:/PROGRA~2/MIB055~1/2019/COMMUN~1/VC/Tools/MSVC/1429~1.300/Include\sstream:11:
In file included from C:/PROGRA~2/MIB055~1/2019/COMMUN~1/VC/Tools/MSVC/1429~1.300/Include\istream:11:
In file included from C:/PROGRA~2/MIB055~1/2019/COMMUN~1/VC/Tools/MSVC/1429~1.300/Include\ostream:11:
In file included from C:/PROGRA~2/MIB055~1/2019/COMMUN~1/VC/Tools/MSVC/1429~1.300/Include\ios:11:
In file included from C:/PROGRA~2/MIB055~1/2019/COMMUN~1/VC/Tools/MSVC/1429~1.300/Include\xlocnum:16:
In file included from C:/PROGRA~2/MIB055~1/2019/COMMUN~1/VC/Tools/MSVC/1429~1.300/Include\streambuf:11:
In file included from C:/PROGRA~2/MIB055~1/2019/COMMUN~1/VC/Tools/MSVC/1429~1.300/Include\xiosbase:12:
In file included from C:/PROGRA~2/MIB055~1/2019/COMMUN~1/VC/Tools/MSVC/1429~1.300/Include\system_error:14:
In file included from C:/PROGRA~2/MIB055~1/2019/COMMUN~1/VC/Tools/MSVC/1429~1.300/Include\stdexcept:12:
C:/PROGRA~2/MIB055~1/2019/COMMUN~1/VC/Tools/MSVC/1429~1.300/Include\xstring(4971,12): error: non-const lvalue reference to type 'basic_istream<...>' cannot bind to a temporary of type 'basic_istream<...>'
return _Istr;
^~~~~
C:/PROGRA~2/MIB055~1/2019/COMMUN~1/VC/Tools/MSVC/1429~1.300/Include\xstring(4977,29): note: in instantiation of function template specialization 'std::operator>><char, std::char_traits<char>, std::allocator<char>>' requested here
return _STD move(_Istr) >> _Str;
^
C:/lo-clang/core/l10ntools/source/merge.cxx(125,18): note: in instantiation of function template specialization 'std::operator>><char, std::char_traits<char>, std::allocator<char>>' requested here
aInputStream >> sPoFile;
^
1 error generated.
make[1]: *** [C:/lo-clang/core/solenv/gbuild/LinkTarget.mk:301: C:/lo-clang/core/workdir/CxxObject/l10ntools/source/merge.o] Error 1In both cases, the reason is that library happens to implement the overload taking basic_istream& by forwarding to the overload taking basic_istream&& (and not the other way around, as libc++ and libstdc++ happen to do), and the fix is to replace
return _Istr;
in the overloads taking basic_istream&& _Istr with
return static_cast<basic_istream<_Elem, _Traits>&>(_Istr);
(No idea if any MSVC standard library developers are around here, who might want to be CC'ed on this.)
Comment Actions
Thank you again @sberg.
I have talked to the MSVC STL maintainers. They would be open to a pull request for fixing this.
The repo can be found at https://github.com/microsoft/STL
Be my guest if you want to submit that fix yourself.
If not, I can probably do it in the near future.
Comment Actions
Thanks a lot for the link. Filed https://github.com/microsoft/STL/pull/2025 "Adapt some return statements to P2266R1 'Simpler implicit move'" now.
Comment Actions
This change has made this snippet fail.
https://godbolt.org/z/3ehK784hY Pass
https://godbolt.org/z/9q48WvsP7 fails.
Comment Actions
Hello!
That is expected breakage from the changes proposed in P2266.
With simpler implicit move, we no longer have a fallback second overload resolution where the return expression would be an l-value and would thus bind to that non-const copy constructor.
You can work that around by introducing an explicit cast to lvalue reference, like this: https://godbolt.org/z/EhYbhYjch
But is this example a reduction from a real world code base?
The committee wants feedback and we are interested how hard you believe this change affects you.
Comment Actions
This is an example that also appears to be impacted by this change:
struct C {
C();
C(C &c1);
};
void foo()
{
try {
C c1;
throw c1;
}
catch (C c2) {
throw;
}
}https://godbolt.org/z/dvEbv7GKo
I'm not certain if this is as expected of an issue, though. In the original example, C carried state that was set up after initialization but was relying on the fallback to the non-idiomatic copy constructor when doing the throw. WDYT?
Comment Actions
I'm not certain it's reasonable to wait for MSVC STL to change as that leaves every existing user of older MSVC STLs out in the cold. This is causing some significant regressions at Intel, to the extent that I wonder if this should be temporarily reverted until the MSVC STL headers can be compiled again.
Comment Actions
Yeah that is the equivalent scenario for throw, we treat c1 as a temporary there. The same workaround applies, you can static cast to non-const lvalue reference.
As far as the implementation is concerned, it is following the proposal to the letter here.
And as I understand it, although I am not the author of the proposal (which is @Quuxplusone), the committee agrees that this breakage is a good thing.
That does interfere with the wishes of the committee to get implementation experience with this.
I am not saying one way or another, but would leaving this effect on by default, but providing a command line flag to turn it off, be a reasonable option on the table?
Comment Actions
Thanks for verifying!
As far as the implementation is concerned, it is following the proposal to the letter here.
And as I understand it, although I am not the author of the proposal (which is @Quuxplusone), the committee agrees that this breakage is a good thing.
I'm less convinced than the committee; I put a high value on existing, working, conforming code continuing to work in newer language modes. However, this is what the standard requires for that language mode, so I can live with it unless the code pattern shows up in some common system header that users can't change themselves, and I haven't run into evidence of that for this particular case yet.
That does interfere with the wishes of the committee to get implementation experience with this.
I would argue it gives the committee valuable implementation experience feedback: the change breaks at least one popular system header.
I am not saying one way or another, but would leaving this effect on by default, but providing a command line flag to turn it off, be a reasonable option on the table?
I don't think it's reasonable to leave this on by default for MSVC compatibility; it's not like std::ifstream is an obscure part of the STL or there's only one version of the STL that's impacted. The suggestions I have for how to move forward are:
- Add a compatibility hack that recognizes using the STL as a system header in a limited way to disable the diagnostics and get the old behavior (we do this on occasion for libstdc++, no reason we couldn't do something similar for MSVC STL). Then old headers still work without needing a flag and new (fixed) headers never see the issue. Someday in the distant future, we can remove the compatibility hack.
- Or, add a command line flag that defaults off for -fms-compatibility and defaults on otherwise. The default can be switched based on the compatibility version being used (perhaps) once the STL has been fixed.
I'd prefer to keep existing code working without requiring users to enable a flag, if at all possible (esp because I don't expect the flag to be needed forever). That said, I still think it makes sense to temporarily revert this while doing that work.
Comment Actions
But we already knew that :-)
Reverting the change globally would stop us from finding further affected entities.
But it is a given that because of this STL bug, this is affecting our ability to get that feedback on user code in that ecosystem as well.
I don't think it's reasonable to leave this on by default for MSVC compatibility; it's not like std::ifstream is an obscure part of the STL or there's only one version of the STL that's impacted. The suggestions I have for how to move forward are:
- Add a compatibility hack that recognizes using the STL as a system header in a limited way to disable the diagnostics and get the old behavior (we do this on occasion for libstdc++, no reason we couldn't do something similar for MSVC STL). Then old headers still work without needing a flag and new (fixed) headers never see the issue. Someday in the distant future, we can remove the compatibility hack.
- Or, add a command line flag that defaults off for -fms-compatibility and defaults on otherwise. The default can be switched based on the compatibility version being used (perhaps) once the STL has been fixed.
I find interesting the idea of combining these two options. We can suppress the effect when compiling with fms-compatibility but only on the STL headers themselves.
I'd prefer to keep existing code working without requiring users to enable a flag, if at all possible (esp because I don't expect the flag to be needed forever). That said, I still think it makes sense to temporarily revert this while doing that work.
I'd like to give some time for other stakeholders to give their opinion if this is not too urgent, specially @Quuxplusone and @rsmith.
But thinking of another option, almost as easy as a global revert for this, we could suppress the effect under -fms-compatibility for now, and then implement a more targetted fix where we only suppress this when parsing the STL system headers themselves under fms-compatibility.
Comment Actions
I'd like to give some time for other stakeholders to give their opinion if this is not too urgent, specially @Quuxplusone and @rsmith.
I have no strong/well-informed opinions here. I have an idea to offer, if there is precedent for it. My idea is, keep it as an error if the rvalue resolution finds nothing, but (1) fall back to an lvalue resolution for error-recovery purposes, and (2) suppress the error in system headers. Then, the usage in MSVC STL will keep working but only because it's in a system header. I seem to recall that there is precedent for diagnostics that are "non-downgradable error in user code, yet still suppressed in system headers." If I'm imagining things, then please do not invent such a facility purely for this feature.
My second choice is to do whatever minimalistic hack suffices to satisfy the customer (Aaron etc. in this case), but only with the understanding that we plan to eliminate that hack as soon as MSVC STL gets updated. My third choice is to revert the whole thing and then land a new version of it that always falls back to lvalue resolution, but with an on-by-default warning (not error) (so this would probably affect a ton of your test cases); then the user can downgrade that warning as a workaround.
Comment Actions
That would still leave the effect of changing the deduced type of a decltype(auto) returning function. But I am on the same boat here in that I am not sure if you are imagining things as well :)
My second choice is to do whatever minimalistic hack suffices to satisfy the customer (Aaron etc. in this case), but only with the understanding that we plan to eliminate that hack as soon as MSVC STL gets updated. My third choice is to revert the whole thing and then land a new version of it that always falls back to lvalue resolution, but with an on-by-default warning (not error) (so this would probably affect a ton of your test cases); then the user can downgrade that warning as a workaround.
Again, the change in flow where we make the expression an xvalue early and definitively before return type deduction occurs makes things more complicated.
My vote is the second choice, as I think making a workaround where we temporarily do the C++20 thing only in some contexts is far simpler, specially if we don't have to update a ton of test cases to cover it :)
Comment Actions
@aaron.ballman I created a DR for the option of temporarily disabling the effects of this change under "-fms-compatibility".
https://reviews.llvm.org/D105518
Comment Actions
This makes clang-cl basically unusable for a significant portion of people (enough so that I'm asking for a temporary revert) and I don't think we should continue to leave trunk in a broken state while deciding what to do. Our usual community approach is to revert out changes that cause significant breakage because we always want people (such as downstreams) to be able to track against trunk. That said, based on below, hold off on reversion as I may have misunderstood the fix patch (see below) and that may be the quicker and less disruptive approach.
But thinking of another option, almost as easy as a global revert for this, we could suppress the effect under -fms-compatibility for now, and then implement a more targetted fix where we only suppress this when parsing the STL system headers themselves under fms-compatibility.
Ah! I missed that context in the other patch, thank you for this! I'll go comment on the other review so we keep the discussion in one place, but that makes me more comfortable with the approach from that review.