Allow 'static' storage specifier on an out-of-line class member template declaration in MSVCCompat mode

[Manna]

Summary

Microsoft compiler permits the use of "static" specifier on out-of-class member template declaration.

This patch allows 'static' storage specifier on an out-of-line class member template declaration with a warning in Clang (To be compatible with Microsoft).

Intel C++ compiler allows the 'static' keyword with a warning in Microsoft mode. GCC allows this with a warning with -fpermissive.

[aaron.ballman]

I feel like I must be missing something, because MSVC does not accept the code from your test case: https://godbolt.org/z/r6evxY Are there other language options required to get MSVC to accept this code, or perhaps is it only accepted by certain versions of MSVC?

lib/Sema/SemaDecl.cpp

8637

Can you reverse the order here so it checks MSVCCompat first?

test/SemaCXX/warn-static-outside-class-definition.cpp

12–15

Formatting is off here, you should run the patch through clang-format.

[Manna]

Patch is updated based on the comments.

[Manna]

In D59280#1428991, @aaron.ballman wrote:

I feel like I must be missing something, because MSVC does not accept the code from your test case: https://godbolt.org/z/r6evxY Are there other language options required to get MSVC to accept this code, or perhaps is it only accepted by certain versions of MSVC?

Thanks Aaron.

Yes, it seems like I made some mistake during testing. MSVC2013 accepts the code. MSVS2017 and MSVC2019 fails to compile the same code.

[aaron.ballman]

In D59280#1463768, @Manna wrote:

In D59280#1428991, @aaron.ballman wrote:

I feel like I must be missing something, because MSVC does not accept the code from your test case: https://godbolt.org/z/r6evxY Are there other language options required to get MSVC to accept this code, or perhaps is it only accepted by certain versions of MSVC?

Thanks Aaron.

Yes, it seems like I made some mistake during testing. MSVC2013 accepts the code. MSVS2017 and MSVC2019 fails to compile the same code.

I double-checked MSVC 2015 and it also does not accept the code. Aside from a testing nit, this LGTM!

test/SemaCXX/warn-static-outside-class-definition.cpp

1

You should add a second RUN line and explicitly specify the MSVC versions so that you can test the diagnostic triggering and not triggering, as appropriate.

[Manna]

Sorry for replying late. Thanks for the review. I have updated the patch. Lit test gives warning diagnostic in Microsoft mode up to MSVC 2013 and errors in MSVS2015, MSVS2017 and MSVC2019.

[Manna]

test/SemaCXX/warn-static-outside-class-definition.cpp

1

Fix the issue by updating the patch.

[aaron.ballman]

I would still like to see the second RUN line added to the test so we can ensure we don't regress the behavior.

[Manna]

In D59280#1489086, @aaron.ballman wrote:

I would still like to see the second RUN line added to the test so we can ensure we don't regress the behavior.

Thanks Aaron. I have added second RUN line and explicitly specify the MSVC versions.

[aaron.ballman]

test/SemaCXX/warn-static-outside-class-definition.cpp

2

This isn't quite what I was after. I was looking for a RUN line with a MS version < MSVC 2015 to test that the diagnostic is not triggered in that case.

[Manna]

I have added RUN line (-fms-compatibility-version=12.0) with a MS version < MSVC 2015

[aaron.ballman]

test/SemaCXX/warn-static-outside-class-definition.cpp

2

Something seems amiss here if this test still passes -- I thought one of these RUN lines was going to err and the other was going to warn because the MSVC behavior changed with 2015 for this case?

[Manna]

test/SemaCXX/warn-static-outside-class-definition.cpp

2

This is what I notice with the current patches. The test behavior is changing based upon MSVC version changes.

The test is generating errors with MSVC2015, MSVC2017, and MSVC2019 with but warning with MSVC2013 for this case.

//MSVC2013

ksh-3.2$ clang -c test.cpp
test.cpp:8:23: warning: 'static' can only be specified inside the class definition [-Wmicrosoft-template]
template <typename T> static int C::foo(T) {

^~~~~~~

test.cpp:17:20: warning: 'static' can only be specified inside the class definition [-Wmicrosoft-template]
template <class T> static void S<T>::f() {

^~~~~~~

2 warnings generated.

ksh-3.2$ cl -v
Microsoft (R) C/C++ Optimizing Compiler Version 18.00.40629 for x64
Copyright (C) Microsoft Corporation. All rights reserved.

cl : Command line warning D9002 : ignoring unknown option '-v'
cl : Command line error D8003 : missing source filename

//MSVC2015
ksh-3.2$ clang -c test.cpp
test.cpp:8:23: warning: 'static' can only be specified inside the class definition [-Wmicrosoft-template]
template <typename T> static int C::foo(T) {

^~~~~~~

test.cpp:17:20: error: 'static' can only be specified inside the class definition
template <class T> static void S<T>::f() {

^~~~~~~

1 warning and 1 error generated.

ksh-3.2$ cl -v
Microsoft (R) C/C++ Optimizing Compiler Version 19.00.24210 for x64
Copyright (C) Microsoft Corporation. All rights reserved.

cl : Command line warning D9002 : ignoring unknown option '-v'
cl : Command line error D8003 : missing source filename

//MSVC2017

ksh-3.2$ clang -c test.cpp
test.cpp:8:23: warning: 'static' can only be specified inside the class definition [-Wmicrosoft-template]
template <typename T> static int C::foo(T) {

^~~~~~~

test.cpp:17:20: error: 'static' can only be specified inside the class definition
template <class T> static void S<T>::f() {

^~~~~~~

1 warning and 1 error generated.

ksh-3.2$ cl -v
Microsoft (R) C/C++ Optimizing Compiler Version 19.15.26726 for x64
Copyright (C) Microsoft Corporation. All rights reserved.

cl : Command line warning D9002 : ignoring unknown option '-v'
cl : Command line error D8003 : missing source filename

//MSVC2019
ksh-3.2$ clang -c test.cpp
test.cpp:8:23: warning: 'static' can only be specified inside the class definition [-Wmicrosoft-template]
template <typename T> static int C::foo(T) {

^~~~~~~

test.cpp:17:20: error: 'static' can only be specified inside the class definition
template <class T> static void S<T>::f() {

^~~~~~~

1 warning and 1 error generated.

ksh-3.2$ cl -v
Microsoft (R) C/C++ Optimizing Compiler Version 19.20.27508.1 for x64
Copyright (C) Microsoft Corporation. All rights reserved.

cl : Command line warning D9002 : ignoring unknown option '-v'
cl : Command line error D8003 : missing source filename

[aaron.ballman]

test/SemaCXX/warn-static-outside-class-definition.cpp

2

This behavior points out the issue:

//MSVC2015
ksh-3.2$ clang -c test.cpp
test.cpp:8:23: warning: 'static' can only be specified inside the class definition [-Wmicrosoft-template]
template <typename T> static int C::foo(T) {

^~~~~~~
test.cpp:17:20: error: 'static' can only be specified inside the class definition
template <class T> static void S<T>::f() {

^~~~~~~
1 warning and 1 error generated.

Your test case currently expects a warning line 17 and not an error, so running this test through lit should fail until you update the comment on line 18. That should be a warning in some modes and an error in others.

[Manna]

I have updated test.

//MSVC2013

ksh-3.2$ clang -c test.cpp
test.cpp:8:23: warning: 'static' can only be specified inside the class definition [-Wmicrosoft-template]
template <typename T> static int C::foo(T) {

^~~~~~~

test.cpp:21:20: warning: 'static' can only be specified inside the class definition [-Wmicrosoft-template]
template <class T> static void S<T>::f() {}

^~~~~~~

2 warnings generated.
//MSVC2015
ksh-3.2$ clang -c test.cpp
test.cpp:8:23: warning: 'static' can only be specified inside the class definition [-Wmicrosoft-template]
template <typename T> static int C::foo(T) {

^~~~~~~

test.cpp:18:20: error: 'static' can only be specified inside the class definition
template <class T> static void S<T>::f() {}

^~~~~~~

1 warning and 1 error generated.

[aaron.ballman]

Thank you for updating the test! One minor nit with the way the test case is written, but otherwise LGTM.

test/SemaCXX/warn-static-outside-class-definition.cpp

16–22

I'd prefer to see this written as:

template <class T> static void S<T>::f() {}
#if _MSC_VER >= 1900
  //expected-error@-2 {{'static' can only be specified inside the class definition}}
#else
  //expected-warning@-4 {{'static' can only be specified inside the class definition}}
#endif

[Manna]

Thanks for reviewing. I have updated the test the way it was written before.

[Manna]

In D59280#1493980, @aaron.ballman wrote:

Thank you for updating the test! One minor nit with the way the test case is written, but otherwise LGTM.

Thanks Aaron. I don't think I have commit right. Could you please submit the patches?

Thanks,
Soumi

[aaron.ballman]

In D59280#1494036, @Manna wrote:

In D59280#1493980, @aaron.ballman wrote:

Thank you for updating the test! One minor nit with the way the test case is written, but otherwise LGTM.

Thanks Aaron. I don't think I have commit right. Could you please submit the patches?

Happy to do so -- I've commit in r360250. Thank you for the patch!

[Manna]

In D59280#1494952, @aaron.ballman wrote:

In D59280#1494036, @Manna wrote:

In D59280#1493980, @aaron.ballman wrote:

Thank you for updating the test! One minor nit with the way the test case is written, but otherwise LGTM.

Thanks Aaron. I don't think I have commit right. Could you please submit the patches?

Happy to do so -- I've commit in r360250. Thank you for the patch!

Thank you so much!

lib/Sema/SemaDecl.cpp

if (SC == SC_Static && isa<CXXMethodDecl>(NewFD) &&
       //   A data or function member of a class may be declared static
       //   in a class definition, in which case it is a static member of
       //   the class.
 
       // Complain about the 'static' specifier if it's on an out-of-line
       // member function definition.
 
       // MSVC permits the use of a 'static' storage specifier on an out-of-line
-      // member function template declaration, warn about this.
+      // member function or class member template declaration, warn about this.
       Diag(D.getDeclSpec().getStorageClassSpecLoc(),
-           NewFD->getDescribedFunctionTemplate() && getLangOpts().MSVCCompat
+           (cast<CXXRecordDecl>(DC)->getDescribedClassTemplate() ||
+            NewFD->getDescribedFunctionTemplate()) && getLangOpts().MSVCCompat
            ? diag::ext_static_out_of_line : diag::err_static_out_of_line)
         << FixItHint::CreateRemoval(D.getDeclSpec().getStorageClassSpecLoc());
     }
 
     // C++11 [except.spec]p15:
     //   A deallocation function with no exception-specification is treated
     //   as if it were specified with noexcept(true).
     const FunctionProtoType *FPT = R->getAs<FunctionProtoType>();

test/SemaCXX/warn-static-outside-class-definition.cpp

 // RUN: %clang_cc1 -fms-compatibility -fsyntax-only -verify %s
 
 struct C {
   template <typename T> static int foo(T);
 };
 
 template <typename T> static int C::foo(T) { 
   //expected-warning@-1 {{'static' can only be specified inside the class definition}}
   return 0;
 }
 
+template<class T> struct S { void f(); };
+
+template<class T> static void S<T>::f() {}
+  //expected-warning@-1 {{'static' can only be specified inside the class definition}}