Revert "[Clang] Fix missed initializer instantiation bug for variable templates" #140930
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
… templat…" This reverts commit 0b553e0.
llvmbot
added
clang
Member
|
@llvm/pr-subscribers-clang Author: Paul Kirth (ilovepi) ChangesReverts llvm/llvm-project#138122 The patch causes a regression and prevents compiling valid C++ code. Full diff: https://github.com/llvm/llvm-project/pull/140930.diff 4 Files Affected:
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 648b32c659b4f..941b9ab45cf1c 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -743,7 +743,6 @@ Bug Fixes to C++ Support
in a ``constexpr`` function. (#GH131432)
- Fixed an incorrect TreeTransform for calls to ``consteval`` functions if a conversion template is present. (#GH137885)
- Clang now emits a warning when class template argument deduction for alias templates is used in C++17. (#GH133806)
-- Fix missed initializer instantiation bug for variable templates. (#GH138122)
- Fix a crash when checking the template template parameters of a dependent lambda appearing in an alias declaration.
(#GH136432), (#GH137014), (#GH138018)
- Fixed an assertion when trying to constant-fold various builtins when the argument
diff --git a/clang/lib/Sema/SemaTemplateInstantiateDecl.cpp b/clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
index d1f313e9cb487..7fbda2a804d75 100644
--- a/clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
+++ b/clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
@@ -6032,11 +6032,11 @@ void Sema::BuildVariableInstantiation(
Context.setStaticLocalNumber(NewVar, Context.getStaticLocalNumber(OldVar));
// Figure out whether to eagerly instantiate the initializer.
- if (NewVar->getType()->isUndeducedType()) {
+ if (InstantiatingVarTemplate || InstantiatingVarTemplatePartialSpec) {
+ // We're producing a template. Don't instantiate the initializer yet.
+ } else if (NewVar->getType()->isUndeducedType()) {
// We need the type to complete the declaration of the variable.
InstantiateVariableInitializer(NewVar, OldVar, TemplateArgs);
- } else if (InstantiatingVarTemplate || InstantiatingVarTemplatePartialSpec) {
- // We're producing a template. Don't instantiate the initializer yet.
} else if (InstantiatingSpecFromTemplate ||
(OldVar->isInline() && OldVar->isThisDeclarationADefinition() &&
!NewVar->isThisDeclarationADefinition())) {
diff --git a/clang/test/CodeGenCXX/cxx1z-inline-variables.cpp b/clang/test/CodeGenCXX/cxx1z-inline-variables.cpp
index 1cb30b178e06f..812e438f30c9a 100644
--- a/clang/test/CodeGenCXX/cxx1z-inline-variables.cpp
+++ b/clang/test/CodeGenCXX/cxx1z-inline-variables.cpp
@@ -1,19 +1,5 @@
// RUN: %clang_cc1 -std=c++1z %s -emit-llvm -o - -triple x86_64-linux-gnu | FileCheck %s
-template <typename T> struct InlineAuto {
- template <typename G> inline static auto var = 5;
-};
-int inlineauto = InlineAuto<int>::var<int>;
-// CHECK: @_ZN10InlineAutoIiE3varIiEE = {{.*}}i32 5{{.*}}comdat
-//
-template <typename> struct PartialInlineAuto {
- template <typename, typename> inline static auto var = 6;
- template <typename T> inline static auto var<int, T> = 7;
-};
-
-int partialinlineauto = PartialInlineAuto<int>::var<int, int>;
-// CHECK: @_ZN17PartialInlineAutoIiE3varIiiEE = {{.*}}i32 7{{.*}}comdat
-
struct Q {
// CHECK: @_ZN1Q1kE = linkonce_odr constant i32 5, comdat
static constexpr int k = 5;
diff --git a/clang/test/SemaTemplate/cxx17-inline-variables.cpp b/clang/test/SemaTemplate/cxx17-inline-variables.cpp
index 06f1ec17fb7a8..7fc0aa8eeeb0c 100644
--- a/clang/test/SemaTemplate/cxx17-inline-variables.cpp
+++ b/clang/test/SemaTemplate/cxx17-inline-variables.cpp
@@ -27,15 +27,3 @@ template <typename T> constexpr int A<T>::n = sizeof(A) + sizeof(T);
template <typename T> inline constexpr int A<T>::m = sizeof(A) + sizeof(T);
static_assert(A<int>().f() == 5);
static_assert(A<int>().g() == 5);
-
-template <typename T> struct InlineAuto {
- template <typename G> inline static auto var = 5;
-};
-
-template <typename> struct PartialInlineAuto {
- template <typename, typename> inline static auto var = 6;
- template <typename T> inline static auto var<int, T> = 7;
-};
-
-int inlineauto = InlineAuto<int>::var<int>;
-int partialinlineauto = PartialInlineAuto<int>::var<int, int>;
|
shafik
reviewed
May 21, 2025
Contributor
Author
|
Well, its not the easiest case to reduce, since there's lots of ways the two errors in question could happen. That code has been accepted by both GCC and clang for some time though, so at the very least we need guidance on what is wrong with it. From what I've been able to tell, instantiation now happens earlier and deduction fails, despite there being a valid deduction guide. @frobtech is more familar w/ this code though and can provide more insight. |
zyn0217
approved these changes
May 22, 2025
Contributor
zyn0217
left a comment
zyn0217
left a comment
There was a problem hiding this comment.
I'll approve it, because I have a similar experience that swapping the deferred instantiation and immediate instantiation order caused a very subtle failure of return type deduction.
However, just as what @AaronBallman suggested, please hold off landing for some time to see if a reduced example could help us pinpoint the underlying issue.
Contributor
Author
|
I've updated #140773 with a reproducer. |
Contributor
|
I'll go ahead and merge this: the eager instantiation of member templates - even it is still dependent - doesnt look right to me In the meantime, I'll look into the case and see if I can figure out a solution |
Collaborator
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/168/builds/12338 Here is the relevant piece of the build log for the reference |
Collaborator
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/208/builds/1359 Here is the relevant piece of the build log for the reference |
Collaborator
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/52/builds/8514 Here is the relevant piece of the build log for the reference |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Reverts #138122
The patch causes a regression and prevents compiling valid C++ code.
The code was accepted by earlier versions of clang and GCC.
See #140773 for details.