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

[mizvekov]

Summary

The Microsoft STL currently has some issues with P2266.
We disable it for now in that mode, but we might come back later with a
more targetted approach.

Signed-off-by: Matheus Izvekov <[e-mail]>

[mizvekov]

.

[aaron.ballman]

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.

[aaron.ballman]

In 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.

[erichkeane]

I agree with Aaron for the need of a test. At one point, we may wish to replace these calls with getLangOpts().isCompatibleWithMSVC or some similar inspection of MSCompatibilityVersion, but I acknowledge we can't do that until we figure out what the correct version IS!

Otherwise this seems like an appropriate temporary solution.

clang/lib/Frontend/InitPreprocessor.cpp

602

This isn't adding a 'tab' character here, is it?

[aaron.ballman]

Ping? Normally I wouldn't bother people so quickly, but this patch is addressing a major regression on trunk that had a revert request. We either need to get this landed ASAP or revert the original commit.

[mizvekov]

@aaron.ballman Yeah don't worry, having dinner, my hobby shift is starting soon though :-)

[mizvekov]

Address review comments.

[mizvekov]

clang/lib/Frontend/InitPreprocessor.cpp

602

No, just four spaces. This was clean on my linter, and it passed all green on the buildbots, which have a clang-format check.

[aaron.ballman]

LGTM, thank you!

clang/lib/Frontend/InitPreprocessor.cpp

if (LangOpts.CPlusPlus20) {
     //Builder.defineMacro("__cpp_coroutines", "201902L");
     Builder.defineMacro("__cpp_designated_initializers", "201707L");
     Builder.defineMacro("__cpp_impl_three_way_comparison", "201907L");
     //Builder.defineMacro("__cpp_modules", "201907L");
     Builder.defineMacro("__cpp_using_enum", "201907L");
   }
   // C++2b features.
   if (LangOpts.CPlusPlus2b) {
+    if (!LangOpts.MSVCCompat)
       Builder.defineMacro("__cpp_implicit_move", "202011L");
     Builder.defineMacro("__cpp_size_t_suffix", "202011L");
   }
   if (LangOpts.Char8)
     Builder.defineMacro("__cpp_char8_t", "201811L");
   Builder.defineMacro("__cpp_impl_destroying_delete", "201806L");
 
   // TS features.
   if (LangOpts.Coroutines)

clang/lib/Sema/SemaStmt.cpp

Sema::NamedReturnInfo Sema::getNamedReturnInfo(Expr *&E, bool ForceCXX2b) {
   // ... the expression is the name of a non-volatile automatic object ...
   const auto *DR = dyn_cast<DeclRefExpr>(E->IgnoreParens());
   if (!DR || DR->refersToEnclosingVariableOrCapture())
     return NamedReturnInfo();
   const auto *VD = dyn_cast<VarDecl>(DR->getDecl());
   if (!VD)
     return NamedReturnInfo();
   NamedReturnInfo Res = getNamedReturnInfo(VD);
+  // FIXME: We supress simpler implicit move here (unless ForceCXX2b is true)
+  //        in msvc compatibility mode just as a temporary work around,
+  //        as the MSVC STL has issues with this change.
+  //        We will come back later with a more targeted approach.
   if (Res.Candidate && !E->isXValue() &&
-      (ForceCXX2b || getLangOpts().CPlusPlus2b)) {
+      (ForceCXX2b ||
+       (getLangOpts().CPlusPlus2b && !getLangOpts().MSVCCompat))) {
     E = ImplicitCastExpr::Create(Context, VD->getType().getNonReferenceType(),
                                  CK_NoOp, E, nullptr, VK_XValue,
                                  FPOptionsOverride());
   }
   return Res;
 }
 
 /// Determine whether the given NRVO candidate variable is move-eligible or

clang/test/SemaCXX/cxx2b-p2266-disable-with-msvc-compat.cpp

+// RUN: %clang_cc1 -std=c++2b -fsyntax-only -fcxx-exceptions                    -verify=new %s
+// RUN: %clang_cc1 -std=c++2b -fsyntax-only -fcxx-exceptions -fms-compatibility -verify=old %s
+// RUN: %clang_cc1 -std=c++20 -fsyntax-only -fcxx-exceptions                    -verify=old %s
+
+// FIXME: This is a test for a temporary workaround where we disable simpler implicit moves
+//        when compiling with -fms-compatibility, because the MSVC STL does not compile.
+//        A better workaround is under discussion.
+//        The test cases here are just a copy from `CXX/class/class.init/class.copy.elision/p3.cpp`,
+//        so feel free to delete this file when the workaround is not needed anymore.
+
+struct CopyOnly {
+  CopyOnly(); // new-note {{candidate constructor not viable: requires 0 arguments, but 1 was provided}}
+  // new-note@-1 {{candidate constructor not viable: requires 0 arguments, but 1 was provided}}
+  CopyOnly(CopyOnly &); // new-note {{candidate constructor not viable: expects an lvalue for 1st argument}}
+  // new-note@-1 {{candidate constructor not viable: expects an lvalue for 1st argument}}
+};
+struct MoveOnly {
+  MoveOnly();
+  MoveOnly(MoveOnly &&);
+};
+MoveOnly &&rref();
+
+MoveOnly &&test1(MoveOnly &&w) {
+  return w; // old-error {{cannot bind to lvalue of type}}
+}
+
+CopyOnly test2(bool b) {
+  static CopyOnly w1;
+  CopyOnly w2;
+  if (b) {
+    return w1;
+  } else {
+    return w2; // new-error {{no matching constructor for initialization}}
+  }
+}
+
+template <class T> T &&test3(T &&x) { return x; } // old-error {{cannot bind to lvalue of type}}
+template MoveOnly &test3<MoveOnly &>(MoveOnly &);
+template MoveOnly &&test3<MoveOnly>(MoveOnly &&); // old-note {{in instantiation of function template specialization}}
+
+MoveOnly &&test4() {
+  MoveOnly &&x = rref();
+  return x; // old-error {{cannot bind to lvalue of type}}
+}
+
+void test5() try {
+  CopyOnly x;
+  throw x; // new-error {{no matching constructor for initialization}}
+} catch (...) {
+}