[Clang]: Diagnose deprecated copy operations also in MSVC compatibility mode

[ningvin]

Summary

When running in MSVC compatibility mode, previously no deprecated copy operation warnings (enabled by -Wdeprecated-copy) were raised. This restriction was already in place when the deprecated copy warning was first introduced.

This patch removes said restriction so that deprecated copy warnings, if enabled, are also raised in MSVC compatibility mode. The reasoning here being that these warnings are still useful when running in MSVC compatibility mode and also have to be semi-explicitly enabled in the first place (using -Wdeprecated-copy, -Wdeprecated or -Wextra).

[dblaikie]

Got any pointers to the commits/authors that added this functionality in the first place? (would be good to have any review history, etc, to check against the change here)

@hansw for windows compatibility discussions

[ningvin]

In D133354#3778237, @dblaikie wrote:

Got any pointers to the commits/authors that added this functionality in the first place? (would be good to have any review history, etc, to check against the change here)

That would be @rsmith if I am not mistaken with https://reviews.llvm.org/rGd577fbbd1c9d6dab193d530fcd807efc3b3bc9ad (not sure how to best link to commits/changes in phabricator).

[dblaikie]

In D133354#3778281, @ningvin wrote:

In D133354#3778237, @dblaikie wrote:

Got any pointers to the commits/authors that added this functionality in the first place? (would be good to have any review history, etc, to check against the change here)

That would be @rsmith if I am not mistaken

Ah, indeed - thanks for the context. (I almost thought it wasn't in that patch, because there was no mention of "MSVC" there - but I see now it was "MicrosoftMode" back then)

with https://reviews.llvm.org/rGd577fbbd1c9d6dab193d530fcd807efc3b3bc9ad (not sure how to best link to commits/changes in phabricator).

That works, you can also take the hash with the rG prefix and use that: rGd577fbbd1c9d6dab193d530fcd807efc3b3bc9ad and that works too.

[hans]

For more context, this was initially raised in https://discourse.llvm.org/t/why-is-the-deprecated-copy-warning-suppressed-in-msvc-compatibility-mode/65085/1

I'm not aware of any reason for the special microsoft mode case, but it's very possible that I'm missing something.

[ningvin]

In D133354#3779123, @dblaikie wrote:

That works, you can also take the hash with the rG prefix and use that: rGd577fbbd1c9d6dab193d530fcd807efc3b3bc9ad and that works too.

Didn't know that, thanks!

In D133354#3779765, @hans wrote:

For more context, this was initially raised in https://discourse.llvm.org/t/why-is-the-deprecated-copy-warning-suppressed-in-msvc-compatibility-mode/65085/1

Oh yes sorry, I should have linked to that.

[dblaikie]

In D133354#3779882, @ningvin wrote:

In D133354#3779123, @dblaikie wrote:

That works, you can also take the hash with the rG prefix and use that: rGd577fbbd1c9d6dab193d530fcd807efc3b3bc9ad and that works too.

Didn't know that, thanks!

In D133354#3779765, @hans wrote:

For more context, this was initially raised in https://discourse.llvm.org/t/why-is-the-deprecated-copy-warning-suppressed-in-msvc-compatibility-mode/65085/1

Oh yes sorry, I should have linked to that.

ah, *thumbs up* then, though I'll probably leave it to @hansw to review so he's aware of when this goes in/will have context in case any fallout is observed on the Windows side of things

[hans]

clang/test/SemaCXX/deprecated-copy-msvc.cpp

1 ↗

(On Diff #458154)

Can we just add a RUN line with -fms-compatibility to clang/test/SemaCXX/deprecated-copy.cpp ?

[ningvin]

That is indeed a better option than to simply copy the test case. I did some more digging and found the following test cases which reference the deprecated copy warning (all located under clang/test/SemaCXX/):

• deprecated.cpp
• deprecated-copy.cpp
• deprecated-copy-with-dtor.cpp
• deprecated-copy-with-user-provided-copy.cpp
• deprecated-copy-with-user-provided-dtor.cpp

I suppose adding the respective RUN lines with -fms-compatibility to all of them is the way to go. I will try that tomorrow to see if something breaks and then update this patch.

[ningvin]

I added the respective RUN lines to the different test cases, did not seem to break anything on my end.

[hans]

lgtm

Maybe give Richard another day or two in case he wants to take a look, but otherwise landing this and seeing if there are any problems we didn't know about sounds good to me.

[ningvin]

@hans just to clarify as I do not have commit access: do I still need to perform some action?

[hans]

In D133354#3789212, @ningvin wrote:

@hans just to clarify as I do not have commit access: do I still need to perform some action?

Ah, thanks for letting me know. I've committed on your behalf as https://github.com/llvm/llvm-project/commit/49e7ef2c09facd722a29a5ad96a7f8f16e362b28

[ningvin]

In D133354#3790064, @hans wrote:

Ah, thanks for letting me know. I've committed on your behalf as https://github.com/llvm/llvm-project/commit/49e7ef2c09facd722a29a5ad96a7f8f16e362b28

Sweet, thanks a lot :-)

clang/lib/Sema/SemaDeclCXX.cpp

 /// which is deprecated because the class has a user-declared copy constructor,
 /// copy assignment operator, or destructor.
 static void diagnoseDeprecatedCopyOperation(Sema &S, CXXMethodDecl *CopyOp) {
   assert(CopyOp->isImplicit());
 
   CXXRecordDecl *RD = CopyOp->getParent();
   CXXMethodDecl *UserDeclaredOperation = nullptr;
 
-  // In Microsoft mode, assignment operations don't affect constructors and
-  // vice versa.
   if (RD->hasUserDeclaredDestructor()) {
     UserDeclaredOperation = RD->getDestructor();
   } else if (!isa<CXXConstructorDecl>(CopyOp) &&
-             RD->hasUserDeclaredCopyConstructor() &&
-             !S.getLangOpts().MSVCCompat) {
+             RD->hasUserDeclaredCopyConstructor()) {
     // Find any user-declared copy constructor.
     for (auto *I : RD->ctors()) {
       if (I->isCopyConstructor()) {
         UserDeclaredOperation = I;
         break;
       }
     }
     assert(UserDeclaredOperation);
   } else if (isa<CXXConstructorDecl>(CopyOp) &&
-             RD->hasUserDeclaredCopyAssignment() &&
-             !S.getLangOpts().MSVCCompat) {
+             RD->hasUserDeclaredCopyAssignment()) {
     // Find any user-declared move assignment operator.
     for (auto *I : RD->methods()) {
       if (I->isCopyAssignmentOperator()) {
         UserDeclaredOperation = I;
         break;
       }
     }
     assert(UserDeclaredOperation);

clang/test/SemaCXX/deprecated-copy-with-dtor.cpp

 // RUN: %clang_cc1 -std=c++11 %s -Wdeprecated -verify
+// RUN: %clang_cc1 -std=c++11 %s -Wdeprecated -verify -fms-compatibility
 // RUN: %clang_cc1 -std=c++11 %s -Wdeprecated-copy-dtor -verify
+// RUN: %clang_cc1 -std=c++11 %s -Wdeprecated-copy-dtor -verify -fms-compatibility
 // RUN: %clang_cc1 -std=c++11 %s -Wdeprecated-copy-with-dtor -verify
+// RUN: %clang_cc1 -std=c++11 %s -Wdeprecated-copy-with-dtor -verify -fms-compatibility
 
 class A {
 public:
    ~A() = default; // expected-warning {{definition of implicit copy constructor for 'A' is deprecated because it has a user-declared destructor}}
 };
 
 void test() {
   A a1;
   A a2 = a1; // expected-note {{in implicit copy constructor for 'A' first required here}}
 }

clang/test/SemaCXX/deprecated-copy-with-user-provided-copy.cpp

 // RUN: %clang_cc1 -std=c++11 %s -Wdeprecated -verify
+// RUN: %clang_cc1 -std=c++11 %s -Wdeprecated -verify -fms-compatibility
 // RUN: %clang_cc1 -std=c++11 %s -Wdeprecated-copy-with-user-provided-copy -verify
+// RUN: %clang_cc1 -std=c++11 %s -Wdeprecated-copy-with-user-provided-copy -verify -fms-compatibility
 
 struct A {
   A &operator=(const A &); // expected-warning {{definition of implicit copy constructor for 'A' is deprecated because it has a user-provided copy assignment operator}}
 };
 
 void foo() {
   A a1;
   A a2(a1); // expected-note {{implicit copy constructor for 'A' first required here}}
 }

clang/test/SemaCXX/deprecated-copy-with-user-provided-dtor.cpp

 // RUN: %clang_cc1 -std=c++11 %s -Wdeprecated -verify
+// RUN: %clang_cc1 -std=c++11 %s -Wdeprecated -verify -fms-compatibility
 // RUN: %clang_cc1 -std=c++11 %s -Wdeprecated-copy-with-user-provided-dtor -verify
+// RUN: %clang_cc1 -std=c++11 %s -Wdeprecated-copy-with-user-provided-dtor -verify -fms-compatibility
 
 struct A {
   ~A(); // expected-warning {{definition of implicit copy constructor for 'A' is deprecated because it has a user-provided destructor}}
 };
 
 void test() {
   A a1;
   A a2(a1); // expected-note {{implicit copy constructor for 'A' first required here}}
 }

clang/test/SemaCXX/deprecated-copy.cpp

 // RUN: %clang_cc1 -std=c++11 %s -Wdeprecated -verify
+// RUN: %clang_cc1 -std=c++11 %s -Wdeprecated -verify -fms-compatibility
 // RUN: %clang_cc1 -std=c++11 %s -Wdeprecated-copy -verify
+// RUN: %clang_cc1 -std=c++11 %s -Wdeprecated-copy -verify -fms-compatibility
 
 struct A {
     A& operator=(const A&) = default; // expected-warning {{definition of implicit copy constructor for 'A' is deprecated because it has a user-declared copy assignment operator}}
 };
 
 struct B {
     B& operator=(const B&) = delete; // expected-warning {{definition of implicit copy constructor for 'B' is deprecated because it has a user-declared copy assignment operator}}
 };

clang/test/SemaCXX/deprecated.cpp

 // RUN: %clang_cc1 -std=c++98 %s -Wno-parentheses -Wdeprecated -verify=expected,not-cxx20 -triple x86_64-linux-gnu
+// RUN: %clang_cc1 -std=c++98 %s -Wno-parentheses -Wdeprecated -verify=expected,not-cxx20 -triple x86_64-linux-gnu -fms-compatibility
 // RUN: %clang_cc1 -std=c++11 %s -Wno-parentheses -Wdeprecated -verify=expected,not-cxx20 -triple x86_64-linux-gnu
+// RUN: %clang_cc1 -std=c++11 %s -Wno-parentheses -Wdeprecated -verify=expected,not-cxx20 -triple x86_64-linux-gnu -fms-compatibility
 // RUN: %clang_cc1 -std=c++14 %s -Wno-parentheses -Wdeprecated -verify=expected,not-cxx20 -triple x86_64-linux-gnu
+// RUN: %clang_cc1 -std=c++14 %s -Wno-parentheses -Wdeprecated -verify=expected,not-cxx20 -triple x86_64-linux-gnu -fms-compatibility
 // RUN: %clang_cc1 -std=c++17 %s -Wno-parentheses -Wdeprecated -verify=expected,not-cxx20 -triple x86_64-linux-gnu
+// RUN: %clang_cc1 -std=c++17 %s -Wno-parentheses -Wdeprecated -verify=expected,not-cxx20 -triple x86_64-linux-gnu -fms-compatibility
 // RUN: %clang_cc1 -std=c++2a %s -Wno-parentheses -Wdeprecated -verify=expected,cxx20 -triple x86_64-linux-gnu
+// RUN: %clang_cc1 -std=c++2a %s -Wno-parentheses -Wdeprecated -verify=expected,cxx20 -triple x86_64-linux-gnu -fms-compatibility
 
 // RUN: %clang_cc1 -std=c++14 %s -Wno-parentheses -Wdeprecated -verify=expected,not-cxx20 -triple x86_64-linux-gnu -Wno-deprecated-register -DNO_DEPRECATED_FLAGS
+// RUN: %clang_cc1 -std=c++14 %s -Wno-parentheses -Wdeprecated -verify=expected,not-cxx20 -triple x86_64-linux-gnu -Wno-deprecated-register -DNO_DEPRECATED_FLAGS -fms-compatibility
 
 #include "Inputs/register.h"
 
 namespace std {
   struct type_info {};
 }
 
 void g() throw();