Repository navigation
[clang][Modules] Complete the implementation of P2615: Meaningful exports - #194201
localspook wants to merge 13 commits into
Conversation
🐧 Linux x64 Test Results
✅ The build succeeded and all tests passed. |
🪟 Windows x64 Test Results
✅ The build succeeded and all tests passed. |
d41a291 to
26d672f
Compare
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
bbaeeaa to
e6afad2
Compare
e6afad2 to
b8f38d5
Compare
llvmbot
added
clang
|
@llvm/pr-subscribers-clang-modules @llvm/pr-subscribers-clang Author: Victor Chernyakin (localspook) ChangesP2615 restricted certain declarations from appearing in an unbraced export-declaration or linkage-specification. This change implements the logic to reject these cases. Fixes #160016. Made possible (or at least quite a bit easier) by #191658! Full diff: https://github.com/llvm/llvm-project/pull/194201.diff 15 Files Affected:
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index c5c8c1fa12e7a..357ef60d8bdc1 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -49,6 +49,10 @@ C++ Specific Potentially Breaking Changes
- Clang now correctly rejects ``export`` declarations in module implementation
partitions. (#GH107602)
+- Clang now correctly rejects explicit instantiations and specializations
+ marked with ``export`` or a language linkage specification, completing
+ its implementation of P2615. (#GH160016)
+
ABI Changes in This Version
---------------------------
diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index 7ed4684c8359e..941d27ce1b11c 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -12854,6 +12854,15 @@ def err_invalid_module_name : Error<"%0 is an invalid name for a module">;
def err_extern_def_in_header_unit : Error<
"non-inline external definitions are not permitted in C++ header units">;
+def err_meaningless_export : Error<
+ "%select{an explicit instantiation|a specialization}0 cannot be "
+ "marked 'export'">;
+def note_meaningless_export_explanation : Note<
+ "as long as its primary template is exported, it will be too">;
+def err_invalid_decl_in_linkage_spec : Error<
+ "language linkage specification cannot be applied to "
+ "%select{an explicit instantiation|a specialization|an export declaration}0">;
+
def warn_exposure : Warning <
"TU local entity %0 is exposed">,
InGroup<DiagGroup<"TU-local-entity-exposure">>;
diff --git a/clang/include/clang/Parse/Parser.h b/clang/include/clang/Parse/Parser.h
index 1d07d8dbcfa01..e4d91451db09e 100644
--- a/clang/include/clang/Parse/Parser.h
+++ b/clang/include/clang/Parse/Parser.h
@@ -3192,7 +3192,7 @@ class Parser : public CodeCompletionHandler {
/// \verbatim
/// linkage-specification: [C++ 7.5p2: dcl.link]
/// 'extern' string-literal '{' declaration-seq[opt] '}'
- /// 'extern' string-literal declaration
+ /// 'extern' string-literal name-declaration
/// \endverbatim
///
Decl *ParseLinkage(ParsingDeclSpec &DS, DeclaratorContext Context);
@@ -3201,7 +3201,7 @@ class Parser : public CodeCompletionHandler {
///
/// \verbatim
/// export-declaration:
- /// 'export' declaration
+ /// 'export' name-declaration
/// 'export' '{' declaration-seq[opt] '}'
/// \endverbatim
///
@@ -3217,6 +3217,29 @@ class Parser : public CodeCompletionHandler {
///
Decl *ParseExportDeclaration();
+ /// Ensure the declaration in an unbraced linkage-specification or
+ /// export-declaration is not an explicit-instantiation,
+ /// explicit-specialization, or export-declaration:
+ ///
+ /// \verbatim
+ /// export-declaration: [C++: module.interface]
+ /// export name-declaration
+ ///
+ /// linkage-specification: [C++: dcl.link]
+ /// export name-declaration
+ ///
+ /// declaration: [C++: dcl.pre]
+ /// name-declaration
+ /// special-declaration
+ ///
+ /// special-declaration: [C++: dcl.pre]
+ /// explicit-instantiation
+ /// explicit-specialization
+ /// export-declaration
+ /// \endverbatim
+ ///
+ void CheckUnbracedLinkageOrExportDeclaration(Decl *LinkageOrExportDecl);
+
/// ParseUsingDirectiveOrDeclaration - Parse C++ using using-declaration or
/// using-directive. Assumes that current token is 'using'.
DeclGroupPtrTy ParseUsingDirectiveOrDeclaration(
diff --git a/clang/lib/Parse/ParseDeclCXX.cpp b/clang/lib/Parse/ParseDeclCXX.cpp
index d1e2e2c2c6ce1..7ee1b7d4d9e75 100644
--- a/clang/lib/Parse/ParseDeclCXX.cpp
+++ b/clang/lib/Parse/ParseDeclCXX.cpp
@@ -339,9 +339,11 @@ Decl *Parser::ParseLinkage(ParsingDeclSpec &DS, DeclaratorContext Context) {
// ... but anyway remember that such an "extern" was seen.
DS.setExternInLinkageSpec(true);
ParseExternalDeclaration(DeclAttrs, DeclSpecAttrs, &DS);
- return LinkageSpec ? Actions.ActOnFinishLinkageSpecification(
- getCurScope(), LinkageSpec, SourceLocation())
- : nullptr;
+ if (!LinkageSpec)
+ return nullptr;
+ CheckUnbracedLinkageOrExportDeclaration(LinkageSpec);
+ return Actions.ActOnFinishLinkageSpecification(getCurScope(), LinkageSpec,
+ SourceLocation());
}
DS.abort();
@@ -420,6 +422,7 @@ Decl *Parser::ParseExportDeclaration() {
MaybeParseCXX11Attributes(DeclAttrs);
ParsedAttributes EmptyDeclSpecAttrs(AttrFactory);
ParseExternalDeclaration(DeclAttrs, EmptyDeclSpecAttrs);
+ CheckUnbracedLinkageOrExportDeclaration(ExportDecl);
return Actions.ActOnFinishExportDecl(getCurScope(), ExportDecl,
SourceLocation());
}
@@ -440,6 +443,52 @@ Decl *Parser::ParseExportDeclaration() {
T.getCloseLocation());
}
+void Parser::CheckUnbracedLinkageOrExportDeclaration(
+ Decl *LinkageOrExportDecl) {
+ const auto *DC = cast<DeclContext>(LinkageOrExportDecl);
+ if (DC->decls_empty())
+ return;
+
+ const Decl *D = *DC->decls_begin();
+
+ // Nested export declarations are diagnosed elsewhere.
+ if (isa<LinkageSpecDecl>(LinkageOrExportDecl) && isa<ExportDecl>(D)) {
+ Diag(LinkageOrExportDecl->getLocation(),
+ diag::err_invalid_decl_in_linkage_spec)
+ << 2;
+ return;
+ }
+
+ TemplateSpecializationKind TSK = [&] {
+ if (const auto *EID = dyn_cast<ExplicitInstantiationDecl>(D))
+ return EID->getTemplateSpecializationKind();
+ if (const auto *CTSD = dyn_cast<ClassTemplateSpecializationDecl>(D))
+ return CTSD->getTemplateSpecializationKind();
+ if (const auto *VTSD = dyn_cast<VarTemplateSpecializationDecl>(D))
+ return VTSD->getTemplateSpecializationKind();
+ if (const auto *FD = dyn_cast<FunctionDecl>(D))
+ return FD->getTemplateSpecializationKind();
+ return TSK_Undeclared;
+ }();
+
+ if (TSK == TSK_Undeclared)
+ return;
+
+ if (const auto *ED = dyn_cast<ExportDecl>(LinkageOrExportDecl)) {
+ Diag(ED->getExportLoc(), diag::err_meaningless_export)
+ << (TSK == TSK_ExplicitSpecialization)
+ << FixItHint::CreateRemoval(ED->getExportLoc());
+ Diag(ED->getExportLoc(), diag::note_meaningless_export_explanation);
+ return;
+ }
+
+ if (const auto *LS = dyn_cast<LinkageSpecDecl>(LinkageOrExportDecl)) {
+ Diag(LS->getLocation(), diag::err_invalid_decl_in_linkage_spec)
+ << (TSK == TSK_ExplicitSpecialization);
+ return;
+ }
+}
+
Parser::DeclGroupPtrTy Parser::ParseUsingDirectiveOrDeclaration(
DeclaratorContext Context, const ParsedTemplateInfo &TemplateInfo,
SourceLocation &DeclEnd, ParsedAttributes &Attrs) {
diff --git a/clang/test/Modules/explicit-specializations.cppm b/clang/test/Modules/explicit-specializations.cppm
index 914144018e880..b5b8d152e3a7b 100644
--- a/clang/test/Modules/explicit-specializations.cppm
+++ b/clang/test/Modules/explicit-specializations.cppm
@@ -20,7 +20,7 @@ struct S {
export struct A {};
-export template <>
+template <>
struct S<A> {
static constexpr bool selected = true;
};
@@ -34,7 +34,7 @@ struct V {
static constexpr bool selected = false;
};
-export template <>
+template<>
struct V<S> {
static constexpr bool selected = true;
};
@@ -46,7 +46,7 @@ struct Numbers {
static constexpr int value = X;
};
-export template<>
+template<>
struct Numbers<43> {
static constexpr bool selected = true;
static constexpr int value = 43;
@@ -58,7 +58,7 @@ struct Pointers {
};
export int IntegralValue = 0;
-export template<>
+template<>
struct Pointers<&IntegralValue> {
static constexpr bool selected = true;
};
@@ -68,7 +68,7 @@ struct NullPointers {
static constexpr bool selected = false;
};
-export template<>
+template<>
struct NullPointers<nullptr> {
static constexpr bool selected = true;
};
@@ -79,7 +79,7 @@ struct Array {
};
export int array[5];
-export template<>
+template<>
struct Array<array> {
static constexpr bool selected = true;
};
diff --git a/clang/test/Modules/export-language-linkage.cppm b/clang/test/Modules/export-language-linkage.cppm
index f389d9604ef3a..e13d0003f248b 100644
--- a/clang/test/Modules/export-language-linkage.cppm
+++ b/clang/test/Modules/export-language-linkage.cppm
@@ -55,7 +55,7 @@ extern "C++" {
int h();
}
-extern "C++" export int g();
+export int g();
//--- d.cpp
import c;
diff --git a/clang/test/Modules/merge-var-template-spec-cxx-modules.cppm b/clang/test/Modules/merge-var-template-spec-cxx-modules.cppm
index db3f4cd518716..e09fcbaa92d95 100644
--- a/clang/test/Modules/merge-var-template-spec-cxx-modules.cppm
+++ b/clang/test/Modules/merge-var-template-spec-cxx-modules.cppm
@@ -28,7 +28,7 @@ template <class T> constexpr T* zero<T*> = nullptr; // expected-error-re {{decla
// expected-note@* {{previous}}
template <> constexpr int** zero<int**> = nullptr; // ok, new specialization.
-template <class T> constexpr T** zero<T**> = nullptr; // ok, new partial specilization.
+template <class T> constexpr T** zero<T**> = nullptr; // ok, new partial specialization.
//--- var_def.cppm
export module var_def;
@@ -37,8 +37,16 @@ export template <class T> constexpr T zero = 0;
export struct Int {
int value;
};
-export template <> constexpr Int zero<Int> = {0};
-export template <class T> constexpr T* zero<T*> = nullptr;
+
+// FIXME: it should make no difference whether a specialization is
+// exported or not, but currently, removing this 'export'
+// leads to an assertion failure in Sema::shouldLinkPossiblyHiddenDecl.
+export {
+
+template <> constexpr Int zero<Int> = {0};
+template <class T> constexpr T* zero<T*> = nullptr;
+
+}
//--- reexport1.cppm
export module reexport1;
diff --git a/clang/test/Modules/pr59780.cppm b/clang/test/Modules/pr59780.cppm
index ea5fdd5c4ce26..8e2685e328133 100644
--- a/clang/test/Modules/pr59780.cppm
+++ b/clang/test/Modules/pr59780.cppm
@@ -25,7 +25,7 @@ export module a;
export template<typename T>
int x = 0;
-export template<>
+template<>
int x<int> = 0;
export template<typename T>
@@ -36,7 +36,7 @@ struct Y {
template <typename T>
int Y<T>::value = 0;
-export template<>
+template<>
struct Y<int> {
static int value;
};
diff --git a/clang/test/Modules/pr60890.cppm b/clang/test/Modules/pr60890.cppm
index b1d9114bf1ebe..5df2f2ea4bdf2 100644
--- a/clang/test/Modules/pr60890.cppm
+++ b/clang/test/Modules/pr60890.cppm
@@ -24,12 +24,12 @@ struct a {
void aaa() requires(true) {}
};
-export template struct a<double>;
+template struct a<double>;
export template<typename T>
void foo(T) requires(true) {}
-export template void foo<double>(double);
+template void foo<double>(double);
export template <typename T>
class A {
diff --git a/clang/test/Modules/pr97313.cppm b/clang/test/Modules/pr97313.cppm
index 99795d6e43030..833e7ff416247 100644
--- a/clang/test/Modules/pr97313.cppm
+++ b/clang/test/Modules/pr97313.cppm
@@ -76,7 +76,7 @@ public:
virtual ~Template();
};
-export template<>
+template<>
class Template<char> {
public:
virtual ~Template();
@@ -84,11 +84,11 @@ public:
// CHECK: @_ZTIW3Mod8TemplateIcE = {{.*}}constant
-export template class Template<unsigned>;
+template class Template<unsigned>;
// CHECK: @_ZTIW3Mod8TemplateIjE = {{.*}}weak_odr
-export extern template class Template<double>;
+extern template class Template<double>;
auto v = new Template<signed int>();
diff --git a/clang/test/Modules/template-function-specialization.cpp b/clang/test/Modules/template-function-specialization.cpp
index d5d7d7e812398..7619f76ab5634 100644
--- a/clang/test/Modules/template-function-specialization.cpp
+++ b/clang/test/Modules/template-function-specialization.cpp
@@ -42,7 +42,7 @@ export template <typename T>
void foo4() {
}
-export template <>
+template <>
void foo4<int>() {
}
diff --git a/clang/test/SemaCXX/P2615.cpp b/clang/test/SemaCXX/P2615.cpp
index fed177c21bc79..947d7e4bf459f 100644
--- a/clang/test/SemaCXX/P2615.cpp
+++ b/clang/test/SemaCXX/P2615.cpp
@@ -3,9 +3,98 @@
// RUN: %clang_cc1 -std=c++20 -verify -fsyntax-only %t/A.cpp
+// RUN: %clang_cc1 -std=c++20 -verify -fsyntax-only %t/B.cpp
//--- A.cpp
// expected-no-diagnostics
export module A;
export namespace N {int x = 42;}
export using namespace N;
+
+//--- B.cpp
+export module B;
+
+export template <typename T> class s1 {};
+export template <typename T> class s1<T *> {}; // expected-error {{a specialization cannot be marked 'export'}}
+// expected-note@-1 {{as long as its primary template is exported, it will be too}}
+export template <> class s1<int> {}; // expected-error {{a specialization cannot be marked 'export'}}
+// expected-note@-1 {{as long as its primary template is exported, it will be too}}
+export template class s1<char>; // expected-error {{an explicit instantiation cannot be marked 'export'}}
+// expected-note@-1 {{as long as its primary template is exported, it will be too}}
+export extern template class s1<void>; // expected-error {{an explicit instantiation cannot be marked 'export'}}
+// expected-note@-1 {{as long as its primary template is exported, it will be too}}
+
+export template <typename T> int v1 = 0;
+export template <typename T> int v1<T *> = 0; // expected-error {{a specialization cannot be marked 'export'}}
+// expected-note@-1 {{as long as its primary template is exported, it will be too}}
+export template <> int v1<int> = 0; // expected-error {{a specialization cannot be marked 'export'}}
+// expected-note@-1 {{as long as its primary template is exported, it will be too}}
+export template int v1<char>; // expected-error {{an explicit instantiation cannot be marked 'export'}}
+// expected-note@-1 {{as long as its primary template is exported, it will be too}}
+export extern template int v1<void>; // expected-error {{an explicit instantiation cannot be marked 'export'}}
+// expected-note@-1 {{as long as its primary template is exported, it will be too}}
+
+export template <typename T> void f1() {}
+export template <> void f1<int>() {} // expected-error {{a specialization cannot be marked 'export'}}
+// expected-note@-1 {{as long as its primary template is exported, it will be too}}
+export template void f1<char>(); // expected-error {{an explicit instantiation cannot be marked 'export'}}
+// expected-note@-1 {{as long as its primary template is exported, it will be too}}
+export extern template void f1<void>(); // expected-error {{an explicit instantiation cannot be marked 'export'}}
+// expected-note@-1 {{as long as its primary template is exported, it will be too}}
+
+
+export { template <typename T> class s2 {}; }
+export { template <typename T> class s2<T *> {}; }
+export { template <> class s2<int> {}; }
+export { template class s2<char>; }
+export { extern template class s2<void>; }
+
+export { template <typename T> int v2 = 0; }
+export { template <typename T> int v2<T *> = 0; }
+export { template <> int v2<int> = 0; }
+export { template int v2<char>; }
+export { extern template int v2<void>; }
+
+export { template <typename T> void f2() {} }
+export { template <> void f2<int>() {} }
+export { template void f2<char>(); }
+export { extern template void f2<void>(); }
+
+
+extern "C++" template <typename T> class s3 {};
+extern "C++" template <typename T> class s3<T *> {}; // expected-error {{language linkage specification cannot be applied to a specialization}}
+extern "C++" template <> class s3<int> {}; // expected-error {{language linkage specification cannot be applied to a specialization}}
+extern "C++" template class s3<char>; // expected-error {{language linkage specification cannot be applied to an explicit instantiation}}
+extern "C++" extern template class s3<void>; // expected-error {{language linkage specification cannot be applied to an explicit instantiation}}
+
+extern "C++" template <typename T> int v3 = 0;
+extern "C++" template <typename T> int v3<T *> = 0; // expected-error {{language linkage specification cannot be applied to a specialization}}
+extern "C++" template <> int v3<int> = 0; // expected-error {{language linkage specification cannot be applied to a specialization}}
+extern "C++" template int v3<char>; // expected-error {{language linkage specification cannot be applied to an explicit instantiation}}
+extern "C++" extern template int v3<void>; // expected-error {{language linkage specification cannot be applied to an explicit instantiation}}
+
+extern "C++" template <typename T> void f3() {}
+extern "C++" template <> void f3<int>() {} // expected-error {{language linkage specification cannot be applied to a specialization}}
+extern "C++" template void f3<char>(); // expected-error {{language linkage specification cannot be applied to an explicit instantiation}}
+extern "C++" extern template void f3<void>(); // expected-error {{language linkage specification cannot be applied to an explicit instantiation}}
+
+extern "C++" export int i; // expected-error {{language linkage specification cannot be applied to an export declaration}}
+extern "C++" export {} // expected-error {{language linkage specification cannot be applied to an export declaration}}
+
+
+extern "C++" { template <typename T> class s4 {}; }
+extern "C++" { template <typename T> class s4<T *> {}; }
+extern "C++" { template <> class s4<int> {}; }
+extern "C++" { template class s4<char>; }
+extern "C++" { extern template class s4<void>; }
+
+extern "C++" { template <typename T> int v4 = 0; }
+extern "C++" { template <typename T> int v4<T *> = 0; }
+extern "C++" { template <> int v4<int> = 0; }
+extern "C++" { template int v4<char>; }
+extern "C++" { extern template int v4<void>; }
+
+extern "C++" { template <typename T> void f4() {} }
+extern "C++" { template <> void f4<int>() {} }
+extern "C++" { template void f4<char>(); }
+extern "C++" { extern template void f4<void>(); }
diff --git a/clang/test/SemaHLSL/Language/groupsharedArgs/ExportNoInlineTest.hlsl b/clang/test/SemaHLSL/Language/groupsharedArgs/ExportNoInlineTest.hlsl
index c120adb4a4fbc..9881b9d43897c 100644
--- a/clang/test/SemaHLSL/Language/groupsharedArgs/ExportNoInlineTest.hlsl
+++ b/clang/test/SemaHLSL/Language/groupsharedArgs/ExportNoInlineTest.hlsl
@@ -15,6 +15,6 @@ void fn3(groupshared T A, groupshared T B) {
A = B;
}
-export template void fn3<uint>(groupshared uint A, groupshared uint B);
+template void fn3<uint>(groupshared uint A, groupshared uint B);
template __attribute__((noinline)) void fn3<float>(groupshared float A, groupshared float B);
// expected-error@-1{{'noinline' attribute is not compatible with 'groupshared' parameter attribute}}
diff --git a/clang/unittests/Serialization/LoadSpecLazilyTest.cpp b/clang/unittests/Serialization/LoadSpecLazilyTest.cpp
index f55925aeae1f2..33fabf451de70 100644
--- a/clang/unittests/Serialization/LoadSpecLazilyTest.cpp
+++ b/clang/unittests/Serialization/LoadSpecLazilyTest.cpp
@@ -238,7 +238,7 @@ export class Temp {
export class ExportedClass {};
-export template<> class A<ExportedClass> {
+template<> class A<ExportedClass> {
A<MayBeLoaded> AS;
A<B> AB;
};
diff --git a/clang/www/cxx_status.html b/clang/www/cxx_status.html
index 3096e55803b84..8a0f3a9b28439 100755
--- a/clang/www/cxx_status.html
+++ b/clang/www/cxx_status.html
@@ -988,7 +988,7 @@ <h2 id="cxx20">C++20 implementation status</h2>
</tr>
<tr> <!-- from Kona 2022 -->
<td><a href="https://wg21.link/P2615R1">P2615R1</a> (<a href="#dr">DR</a>)</td>
- <td class="full" align="center">Clang 17</td>
+ <td class="unreleased" align="center">Clang 23</td>
</tr>
<tr> <!-- from Issaquah 2023 -->
<td><a href="https://wg21.link/P2788R0">P2788R0</a> (<a href="#dr">DR</a>)</td>
|
|
The Windows tests are currently failing because the MSVC STL headers have a lot of template specializations directly marked with |
zwuis
left a comment
zwuis
left a comment
There was a problem hiding this comment.
The Windows tests are currently failing because the MSVC STL headers have a lot of template specializations directly marked with
extern "C++", which is ill-formed under these new rules. That shows this change might be quite disruptive. How should I handle this? Downgrade the error to a warning?
IMO we can suppress diagnostics if we
- detect a macro specific to MSVC STL, and
- the declaration is in namespace
std
like #139560.
|
MSVC STL has fixed most errors, see microsoft/STL#6074. But it seems that MSVC isn't diagnosing |
|
|
||
|
|
||
| extern "C++" template <typename T> class s3 {}; | ||
| extern "C++" template <typename T> class s3<T *> {}; // expected-error {{language linkage specification cannot be applied to a specialization}} |
There was a problem hiding this comment.
IIUC the paper (and the current standard wording) isn't disallowing linkage specification to be applied to partial specializations. Can we avoid rejecting this for now?
There was a problem hiding this comment.
Oh yeah, I see what you mean. Added an exception for this case. I'm wondering whether the difference is intentional...?
There was a problem hiding this comment.
I'm not sure. But the standard requirements are well-defined even with such inconsistency. Perhaps we can submit a following-up CWG issue (https://github.com/cplusplus/CWG/issues) for this if you want.
Added an exclusion for MSVC STL versions with |
ChuanqiXu9
left a comment
ChuanqiXu9
left a comment
There was a problem hiding this comment.
This will introduce breaking changes. I think it is fine enough to emit warning here.
|
Does this PR implement CWG2443? If yes, please add/move some tests to clang/test/CXX/drs, then run clang/www/make_cxx_dr_status. |
As P2615R1 is the paper resolving CWG2443, I don't think it's possible to implement the paper without implementing CWG2443. Given there's already a test file for P2615, I guess it's reasonable to only add new cases to the existing test file and update |
Endilll
left a comment
Endilll
left a comment
There was a problem hiding this comment.
I agree that this PR seems to implement CWG2443. If the author agrees, you should add (but not move) relevant tests to C++ DR tests and claim conformance with CWG2443. I think the scope of CWG2443 is narrower than that of P2615R1: it covers export applied to various declaration, but doesn't cover language linkage specifiers. Please also include test cases with export blocks, which I expect to behave differently than export specifier on declarations.
Also see documentation about C++ defect report tests if you didn't work in clang/test/CXX/drs before — there is a number of special rules upheld there.
Agreed with both of those. I've added tests for CWG2443 by just copy-pasting all the tests involving
These should be there already. |
Do you feel strongly about that? FWIW, GCC and MSVC have both implemented it as an error. (GCC link: https://godbolt.org/z/qPPPz96z9. The MSVC on CE doesn't seem to be new enough to demonstrate this feature, but I tested it locally) |
Yeah, we should avoid breaking changes if possible. It is bad user experience. Especially the forbidden code is not bad actually. |
Endilll
left a comment
Endilll
left a comment
There was a problem hiding this comment.
This introduces a fair bit of duplication though; are we fine with that?
Yes. Regular tests and conformance test can evolve independently (the former — when implementation changes, the latter — when wording changes), so it's always a mistake if a particular aspect of implementation behavior is tested only by conformance tests.
These should be there already.
I meant that they should be included in the CWG2443 test, which you did.
| // on some template specializations, but it would be too disruptive to | ||
| // reject them. This was fixed in | ||
| // https://github.com/microsoft/STL/pull/6074, merged on 2026-02-11. | ||
| bool IsStandardLibrarySymbolInOldMSVCSTL = [&] { |
There was a problem hiding this comment.
Can we introduce something like the following in Preprocessor :
std::optional<llvm::APSInt>
getSpellingOfNumericConstant(const Token &Tok) const {
assert(Tok.is(tok::numeric_constant) && "Called on unsupported token");
SmallString<64> IntegerBuffer;
bool NumberInvalid = false;
StringRef Spelling = getSpelling(Tok, IntegerBuffer, &NumberInvalid);
if (NumberInvalid)
return std::nullopt; // a diagnostic was already reported
NumericLiteralParser Literal(Spelling, Tok.getLocation(),
getSourceManager(), getLangOpts(),
getTargetInfo(), getDiagnostics());
if (Literal.hadError)
return std::nullopt; // a diagnostic was already reported.
if (Literal.isFloatingLiteral() || Literal.isImaginary)
return std::nullopt;
assert(Literal.isIntegerLiteral() && "Unknown ppnumber");
llvm::APSInt Result;
if (Literal.GetIntegerValue(Result))
return std::nullopt; // overflow parsing integer literal.
// Detect overflow based on whether the value is signed.
if (!Literal.isUnsigned && Result.isNegative())
Result.setIsUnsigned(true);
return Result;
}
std::optional<llvm::APSInt> getMsvcStlUpdateConstant() const {
if (getLangOpts().MSVCCompat) {
const MacroInfo *MI = getMacroInfo(getIdentifierInfo("_MSVC_STL_UPDATE"));
if (!MI || MI->getNumTokens() != 1)
return std::nullopt;
return getSpellingOfNumericConstant(MI->getReplacementToken(0));
}
return std::nullopt;
}Then we can check the constant value:
auto Val = getMsvcStlUpdateConstant();
if (Val && *Val < llvm::APSInt(202602L, false) && ...)
...What do you think?
There was a problem hiding this comment.
we have getStdLibCxxVersion for libstdc++, we can extend that for msvc
There was a problem hiding this comment.
Good catch! Yes, We can extend this mechanism.
There was a problem hiding this comment.
Extended the existing mechanism (had to refactor it a bit). PTAL.
| if (DC->decls_empty()) | ||
| return; |
There was a problem hiding this comment.
I don't think that can ever be true, we should assert on it instead
There was a problem hiding this comment.
That causes the following tests to fail:
CXX/module/module.interface/p2-2.cpp
CXX/module/module.interface/p3.cpp
CodeCompletion/keywords-cxx20.cpp
Modules/merge-concepts.cppm
| // There's no equivalent wording for linkage-specification. | ||
| if (isa<ClassTemplatePartialSpecializationDecl, | ||
| VarTemplatePartialSpecializationDecl>(D) && | ||
| isa<LinkageSpecDecl>(LinkageOrExportDecl)) | ||
| return; |
There was a problem hiding this comment.
It is weird that this case is not diagnosed
| // on some template specializations, but it would be too disruptive to | ||
| // reject them. This was fixed in | ||
| // https://github.com/microsoft/STL/pull/6074, merged on 2026-02-11. | ||
| bool IsStandardLibrarySymbolInOldMSVCSTL = [&] { |
There was a problem hiding this comment.
we have getStdLibCxxVersion for libstdc++, we can extend that for msvc
| void ComputeCXXStandardLibraryVersion(); | ||
|
|
||
| bool NeedsCXXStandardLibraryWorkaroundBefore( | ||
| uint64_t FixedVersion, CXXStandardLibraryVersionInfo::Library Lib) { | ||
| ComputeCXXStandardLibraryVersion(); | ||
| return CXXStandardLibraryVersion && CXXStandardLibraryVersion->Lib == Lib && | ||
| CXXStandardLibraryVersion->Version < FixedVersion; | ||
| } |
There was a problem hiding this comment.
I don't think we need these logics in this patch...
| if (const auto *EID = dyn_cast<ExplicitInstantiationDecl>(D)) | ||
| return EID->getTemplateSpecializationKind(); | ||
|
|
||
| return TSK_Undeclared; |
There was a problem hiding this comment.
nit: Slightly odd to return TSK_Undeclared for declaration unrelated to templates.
There was a problem hiding this comment.
Agreed, but it's consistent with all the node-specific getTemplateSpecializationKind methods.
|
A question, why did you implement this in Preprocessor? If I were going to implement this, I am going to implement this in Sema::ActOnFinishExportDecl in clang/lib/Sema/SemaModule.cpp |
(I'm assuming you meant Parser instead of Preprocessor?) That's the easiest place I found to put the implementation so that it can be shared between |
This branch has not been deployed
10 participants
P2615 restricted certain declarations from appearing in an unbraced export-declaration or linkage-specification. This change implements the logic to reject these cases.
Fixes #160016.
Made possible (or at least quite a bit easier) by #191658!