[PATCH] D98904: Instantiate static constexpr data members on MS ABI.

[Zoe Carver via Phabricator via cfe-commits]

zoecarver added a comment.

> So. I think the status quo is OK but not great; we accept invalid code, in 
> the name of MSVC compatibility, that MSVC doesn't accept. I don't think 
> following MSVC would be a good thing, as that'd lead to our rejecting valid 
> code (that MSVC also rejects but that we currently accept). If we can split 
> the difference, and eagerly instantiate only in the case where the language 
> rules say the variable is not inline but MSVC says it is inline, that would 
> be an improvement, but it seems awkward as I think we've already lost the 
> relevant information by the point we need to make the decision.

I agree. And, I think you're right about the fact that we've unfortunately lost 
the relevant info at the point. Even though it's a C++17 extension, someone 
could make `value` inline explicitly (as is done in your Godbolt) and we'd 
still error pre-c++17. Based on that, I'm going to close this for now. But if 
you (or someone else) thinks of another solution, or reason that it would be 
good to move forward with this, I'd be happy to re-open it and continue to work 
on it.

Thanks for all the help with this bug and patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98904

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D98904: Instantiate static constexpr data members on MS ABI.

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

In D98904#2636109 , @zoecarver wrote:

> I think we'd basically need a condition that says: `is-microsoft && 
> less-than-cxx17 && is-constexpr && is-static-data-member`.

Yes.

Looking back over the history a bit here, https://reviews.llvm.org/D47956 
suggests that treating these static constexpr data members as implicitly inline 
matches the MS behavior. And I don't think I can disprove that: considering 
https://godbolt.org/z/7z6T3q, it looks like MSVC gets the "don't instantiate 
the initializer of an inline static data member with the class" rule wrong in 
general, which (perhaps by chance alone) means that MSVC doesn't have the same 
accepts-invalid bug that we have, but only because it's being hidden by a 
rejects-valid bug for the same case!

So. I think the status quo is OK but not great; we accept invalid code, in the 
name of MSVC compatibility, that MSVC doesn't accept. I don't think following 
MSVC would be a good thing, as that'd lead to our rejecting valid code (that 
MSVC also rejects but that we currently accept). If we can split the 
difference, and eagerly instantiate only in the case where the language rules 
say the variable is not inline but MSVC says it is inline, that would be an 
improvement, but it seems awkward as I think we've already lost the relevant 
information by the point we need to make the decision.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98904

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D98904: Instantiate static constexpr data members on MS ABI.

[Zoe Carver via Phabricator via cfe-commits]

zoecarver added a comment.

> (I think you're missing something here to trigger the instantiation of the 
> class, such as Invalid x;.)

Yes, you're right.

> The above example is valid in C++17 onwards, because in C++17 onwards, static 
> constexpr data members are implicitly inline, and the delayed instantiation 
> behavior here is correct for inline static data members. We'll need to 
> distinguish here between the case where the variable is implicitly inline 
> solely because of the MS ABI rules and the case where the variable is inline 
> because of the actual language rules (where it's declared either inline or 
> constexpr).

Hmm. This is interesting. Thanks for explaining. Because this patch isn't going 
to help solve the Swift interop issue anymore, I don't feel a need to continue 
with it. However, if you still think it would be a valuable change to make, I'm 
happy to continue working on it.

I think we'd basically need a condition that says: `is-microsoft && 
less-than-cxx17 && is-constexpr && is-static-data-member`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98904

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D98904: Instantiate static constexpr data members on MS ABI.

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

In D98904#2636021 , @zoecarver wrote:

> So, as I mentioned in the description of this patch, the reason that these 
> members are treated as definitions is that Clang implicitly marks them as 
> inline when targeting MS. This is sort of interesting a) because it only 
> applies to constexpr vars, and b) because this also happens on all platforms 
> for `std=c++17`. This means that the following program will compile 
> successfully with `std=c++17`:
>
>   template 
>   struct GetTypeValue {
> static constexpr const bool value = T::value;
>   };
>   
>   using Invalid = GetTypeValue;

(I think you're missing something here to trigger the instantiation of the 
class, such as `Invalid x;`.)

> However, if the `constexpr` is removed, it will fail to compile. Conversely, 
> after this patch is applied, that will not happen on Windows (it will fail to 
> compile regardless of the targeted standard or constexprness). I think it 
> would make sense to "fix" this for `std=c++17` as well, but I'm not sure if 
> that's in line with the standard.

The above example is valid in C++17 onwards, because in C++17 onwards, static 
constexpr data members are implicitly inline, and the delayed instantiation 
behavior here is correct for inline static data members. We'll need to 
distinguish here between the case where the variable is implicitly inline 
solely because of the MS ABI rules and the case where the variable is inline 
because of the actual language rules (where it's declared either `inline` or 
`constexpr`).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98904

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D98904: Instantiate static constexpr data members on MS ABI.

[Zoe Carver via Phabricator via cfe-commits]

zoecarver added a comment.

So, as I mentioned in the description of this patch, the reason that these 
members are treated as definitions is that Clang implicitly marks them as 
inline when targeting MS. This is sort of interesting a) because it only 
applies to constexpr vars, and b) because this also happens on all platforms 
for `std=c++17`. This means that the following program will compile 
successfully with `std=c++17`:

  template 
  struct GetTypeValue {
static constexpr const bool value = T::value;
  };
  
  using Invalid = GetTypeValue;

However, if the `constexpr` is removed, it will fail to compile. Conversely, 
after this patch is applied, that will not happen on Windows (it will fail to 
compile regardless of the targeted standard or constexprness). I think it would 
make sense to "fix" this for `std=c++17` as well, but I'm not sure if that's in 
line with the standard.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98904

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D98904: Instantiate static constexpr data members on MS ABI.

[Zoe Carver via Phabricator via cfe-commits]

zoecarver created this revision.
zoecarver added a reviewer: rsmith.
zoecarver requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Because MSVC will not treat the definition of a static data member as part of 
the declaration, it will not get instantiated. This means Clang won't diagnose 
instantiation errors until the data member itself is used.

Note: this only happens for constexpr data members because they are the only 
ones that are marked as implicitly inline and therefore not part of the 
declaration.

Refs: https://llvm.org/PR49618
Refs: https://github.com/apple/swift/pull/35962


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D98904

Files:
  clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
  clang/test/SemaCXX/windows-data-member-instantiation-errors.cpp


Index: clang/test/SemaCXX/windows-data-member-instantiation-errors.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/windows-data-member-instantiation-errors.cpp
@@ -0,0 +1,28 @@
+// RUN: %clang_cc1 -triple x86_64-unknown-windows-msvc -verify -std=c++11 %s
+// RUN: %clang_cc1 -triple x86_64-unknown-windows-msvc -verify -std=c++17 %s
+
+template 
+struct GetTypeValue {
+  // expected-error@+1{{type 'int' cannot be used prior to '::' because it has 
no members}}
+  static constexpr const bool value = T::value;
+};
+
+using Invalid = GetTypeValue;
+
+// expected-note@+1{{in instantiation of template class 'GetTypeValue' 
requested here}}
+void test(Invalid) {}
+
+// expected-note@+2{{candidate constructor (the implicit copy constructor) not 
viable: no known conversion from 'int' to 'const S &' for 1st argument}}
+// expected-note@+1{{candidate constructor (the implicit move constructor) not 
viable: no known conversion from 'int' to 'S &&' for 1st argument}}
+struct S {};
+
+template 
+struct MakeS {
+  // expected-error@+1{{no viable conversion from 'int' to 'const S'}}
+  static constexpr const S value = T();
+};
+
+using Invalid2 = MakeS;
+
+// expected-note@+1{{in instantiation of template class 'MakeS' requested 
here}}
+void test2(Invalid2) {}
Index: clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
===
--- clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
+++ clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
@@ -5111,7 +5111,11 @@
 InstantiateVariableInitializer(NewVar, OldVar, TemplateArgs);
   } else if (InstantiatingSpecFromTemplate ||
  (OldVar->isInline() && OldVar->isThisDeclarationADefinition() &&
-  !NewVar->isThisDeclarationADefinition())) {
+  !NewVar->isThisDeclarationADefinition() &&
+  // On Microsoft's ABI, the new var's initializer will be part of
+  // the definition. But, we still want to instanciate it so that
+  // we can catch instantiation errors.
+  !Context.getTargetInfo().getCXXABI().isMicrosoft())) {
 // Delay instantiation of the initializer for variable template
 // specializations or inline static data members until a definition of the
 // variable is needed.


Index: clang/test/SemaCXX/windows-data-member-instantiation-errors.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/windows-data-member-instantiation-errors.cpp
@@ -0,0 +1,28 @@
+// RUN: %clang_cc1 -triple x86_64-unknown-windows-msvc -verify -std=c++11 %s
+// RUN: %clang_cc1 -triple x86_64-unknown-windows-msvc -verify -std=c++17 %s
+
+template 
+struct GetTypeValue {
+  // expected-error@+1{{type 'int' cannot be used prior to '::' because it has no members}}
+  static constexpr const bool value = T::value;
+};
+
+using Invalid = GetTypeValue;
+
+// expected-note@+1{{in instantiation of template class 'GetTypeValue' requested here}}
+void test(Invalid) {}
+
+// expected-note@+2{{candidate constructor (the implicit copy constructor) not viable: no known conversion from 'int' to 'const S &' for 1st argument}}
+// expected-note@+1{{candidate constructor (the implicit move constructor) not viable: no known conversion from 'int' to 'S &&' for 1st argument}}
+struct S {};
+
+template 
+struct MakeS {
+  // expected-error@+1{{no viable conversion from 'int' to 'const S'}}
+  static constexpr const S value = T();
+};
+
+using Invalid2 = MakeS;
+
+// expected-note@+1{{in instantiation of template class 'MakeS' requested here}}
+void test2(Invalid2) {}
Index: clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
===
--- clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
+++ clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
@@ -5111,7 +5111,11 @@
 InstantiateVariableInitializer(NewVar, OldVar, TemplateArgs);
   } else if (InstantiatingSpecFromTemplate ||
  (OldVar->isInline() && OldVar->isThisDeclarationADefinition() &&
-