[PATCH] D99005: [clang] Implement P2266 Simpler implicit move

Tue Jul 6 08:54:42 PDT 2021

aaron.ballman added a comment.

In D99005#2859886 <https://reviews.llvm.org/D99005#2859886>, @mizvekov wrote:

> In D99005#2859476 <https://reviews.llvm.org/D99005#2859476>, @aaron.ballman wrote:
>
>> 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?
>
> 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.

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.

> In D99005#2859652 <https://reviews.llvm.org/D99005#2859652>, @aaron.ballman wrote:
>
>> 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.
>
> 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.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99005