[PATCH] D105518: [clang] disable P2266 simpler implicit moves under -fms-compatibility

Wed Jul 7 04:22:10 PDT 2021

aaron.ballman added a comment.

In D105518#2861635 <https://reviews.llvm.org/D105518#2861635>, @aaron.ballman wrote:

> Thank you for working on this! I applied the patch locally and confirmed that it solves the issue at hand.
>
> However, I'm not certain this is the right approach; what's the plan moving forward? This patch means that users of clang-cl will not get the simpler implicit moves. Once the STL gets fixed, then what? We can't remove this check for MS compat because then people using older STLs will go back to getting unacceptable compile errors and I'd hate to have to wait for long enough that old MSVC STLs have fallen out of circulation before enabling simpler implicit moves for clang-cl users.

I had missed some important context that was in https://reviews.llvm.org/D99005#2860494, namely that this is just to get us back to a good state immediately but there will be further follow-up work to target just the problematic part of the MSVC STL. So long as the follow-up work happens, I think this patch gets us moving forward without causing a lot of churn. If we find that a more targeted approach isn't feasible for some reason, we'll still have to figure out what to do, but at least this enables the feature for users who can use it, which is great.

Can you add a test case to demonstrate the behavior with this patch under `-fms-compatibility` and some comments explaining what's going on (just in case anyone comes peeking while the work is happening)? I'd suggest adding a FIXME comment to SemaStmt.cpp as well, just to be safe, in case the follow-up work is delayed for some reason.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105518