[PATCH] D25777: [Sema][TreeTransform] Re-create DesignatedInitExpr when it has a field designator with a valid FieldDecl
This revision was automatically updated to reflect the committed changes.
arphaman marked an inline comment as done.
Closed by commit rL284959: [Sema][TreeTransform] Re-create DesignatedInitExpr
when a field designator (authored by arphaman).
Changed prior to commit:
https://reviews.llvm.org/D25777?vs=75414&id=75559#toc
Repository:
rL LLVM
https://reviews.llvm.org/D25777
Files:
cfe/trunk/lib/Sema/TreeTransform.h
cfe/trunk/test/SemaCXX/designated-initializers.cpp
Index: cfe/trunk/test/SemaCXX/designated-initializers.cpp
===
--- cfe/trunk/test/SemaCXX/designated-initializers.cpp
+++ cfe/trunk/test/SemaCXX/designated-initializers.cpp
@@ -0,0 +1,31 @@
+// RUN: %clang_cc1 -std=c++11 -fsyntax-only -verify -Winitializer-overrides %s
+
+template struct Foo {
+ struct SubFoo {
+int bar1;
+int bar2;
+ };
+
+ static void Test() { SubFoo sf = {.bar1 = 10, .bar2 = 20}; } // Expected no
warning
+};
+
+void foo() {
+ Foo::Test();
+ Foo::Test();
+ Foo::Test();
+}
+
+template struct Bar {
+ struct SubFoo {
+int bar1;
+int bar2;
+ };
+
+ static void Test() { SubFoo sf = {.bar1 = 10,// expected-note 2
{{previous initialization is here}}
+.bar1 = 20}; } // expected-warning 2
{{initializer overrides prior initialization of this subobject}}
+};
+
+void bar() {
+ Bar::Test(); // expected-note {{in instantiation of member function
'Bar::Test' requested here}}
+ Bar::Test(); // expected-note {{in instantiation of member function
'Bar::Test' requested here}}
+}
Index: cfe/trunk/lib/Sema/TreeTransform.h
===
--- cfe/trunk/lib/Sema/TreeTransform.h
+++ cfe/trunk/lib/Sema/TreeTransform.h
@@ -8923,6 +8923,19 @@
Desig.AddDesignator(Designator::getField(D.getFieldName(),
D.getDotLoc(),
D.getFieldLoc()));
+ if (D.getField()) {
+FieldDecl *Field = cast_or_null(
+getDerived().TransformDecl(D.getFieldLoc(), D.getField()));
+if (Field != D.getField())
+ // Rebuild the expression when the transformed FieldDecl is
+ // different to the already assigned FieldDecl.
+ ExprChanged = true;
+ } else {
+// Ensure that the designator expression is rebuilt when there isn't
+// a resolved FieldDecl in the designator as we don't want to assign
+// a FieldDecl to a pattern designator that will be instantiated again.
+ExprChanged = true;
+ }
continue;
}
Index: cfe/trunk/test/SemaCXX/designated-initializers.cpp
===
--- cfe/trunk/test/SemaCXX/designated-initializers.cpp
+++ cfe/trunk/test/SemaCXX/designated-initializers.cpp
@@ -0,0 +1,31 @@
+// RUN: %clang_cc1 -std=c++11 -fsyntax-only -verify -Winitializer-overrides %s
+
+template struct Foo {
+ struct SubFoo {
+int bar1;
+int bar2;
+ };
+
+ static void Test() { SubFoo sf = {.bar1 = 10, .bar2 = 20}; } // Expected no warning
+};
+
+void foo() {
+ Foo::Test();
+ Foo::Test();
+ Foo::Test();
+}
+
+template struct Bar {
+ struct SubFoo {
+int bar1;
+int bar2;
+ };
+
+ static void Test() { SubFoo sf = {.bar1 = 10,// expected-note 2 {{previous initialization is here}}
+.bar1 = 20}; } // expected-warning 2 {{initializer overrides prior initialization of this subobject}}
+};
+
+void bar() {
+ Bar::Test(); // expected-note {{in instantiation of member function 'Bar::Test' requested here}}
+ Bar::Test(); // expected-note {{in instantiation of member function 'Bar::Test' requested here}}
+}
Index: cfe/trunk/lib/Sema/TreeTransform.h
===
--- cfe/trunk/lib/Sema/TreeTransform.h
+++ cfe/trunk/lib/Sema/TreeTransform.h
@@ -8923,6 +8923,19 @@
Desig.AddDesignator(Designator::getField(D.getFieldName(),
D.getDotLoc(),
D.getFieldLoc()));
+ if (D.getField()) {
+FieldDecl *Field = cast_or_null(
+getDerived().TransformDecl(D.getFieldLoc(), D.getField()));
+if (Field != D.getField())
+ // Rebuild the expression when the transformed FieldDecl is
+ // different to the already assigned FieldDecl.
+ ExprChanged = true;
+ } else {
+// Ensure that the designator expression is rebuilt when there isn't
+// a resolved FieldDecl in the designator as we don't want to assign
+// a FieldDecl to a pattern designator that will be instantiated again.
+ExprChanged = true;
+ }
continue;
}
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/
[PATCH] D25777: [Sema][TreeTransform] Re-create DesignatedInitExpr when it has a field designator with a valid FieldDecl
rjmccall added a comment. Thanks, LGTM with a small tweak. Comment at: lib/Sema/TreeTransform.h:8928 +FieldDecl *Field = dyn_cast_or_null( +getDerived().TransformDecl(D.getFieldLoc(), D.getField())); +if (Field != D.getField()) I'm pretty sure that TransformDecl promises to preserve the decl kind, i.e. this can be a cast_or_null. Repository: rL LLVM https://reviews.llvm.org/D25777 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D25777: [Sema][TreeTransform] Re-create DesignatedInitExpr when it has a field designator with a valid FieldDecl
arphaman updated this revision to Diff 75414.
arphaman added a comment.
The updated patch addresses John's comment by modifying the
`DesignatedInitExpr` re-creation logic.
Repository:
rL LLVM
https://reviews.llvm.org/D25777
Files:
lib/Sema/TreeTransform.h
test/SemaCXX/designated-initializers.cpp
Index: test/SemaCXX/designated-initializers.cpp
===
--- /dev/null
+++ test/SemaCXX/designated-initializers.cpp
@@ -0,0 +1,31 @@
+// RUN: %clang_cc1 -std=c++11 -fsyntax-only -verify -Winitializer-overrides %s
+
+template struct Foo {
+ struct SubFoo {
+int bar1;
+int bar2;
+ };
+
+ static void Test() { SubFoo sf = {.bar1 = 10, .bar2 = 20}; } // Expected no
warning
+};
+
+void foo() {
+ Foo::Test();
+ Foo::Test();
+ Foo::Test();
+}
+
+template struct Bar {
+ struct SubFoo {
+int bar1;
+int bar2;
+ };
+
+ static void Test() { SubFoo sf = {.bar1 = 10,// expected-note 2
{{previous initialization is here}}
+.bar1 = 20}; } // expected-warning 2
{{initializer overrides prior initialization of this subobject}}
+};
+
+void bar() {
+ Bar::Test(); // expected-note {{in instantiation of member function
'Bar::Test' requested here}}
+ Bar::Test(); // expected-note {{in instantiation of member function
'Bar::Test' requested here}}
+}
Index: lib/Sema/TreeTransform.h
===
--- lib/Sema/TreeTransform.h
+++ lib/Sema/TreeTransform.h
@@ -8923,6 +8923,19 @@
Desig.AddDesignator(Designator::getField(D.getFieldName(),
D.getDotLoc(),
D.getFieldLoc()));
+ if (D.getField()) {
+FieldDecl *Field = dyn_cast_or_null(
+getDerived().TransformDecl(D.getFieldLoc(), D.getField()));
+if (Field != D.getField())
+ // Rebuild the expression when the transformed FieldDecl is
+ // different to the already assigned FieldDecl.
+ ExprChanged = true;
+ } else {
+// Ensure that the designator expression is rebuilt when there isn't
+// a resolved FieldDecl in the designator as we don't want to assign
+// a FieldDecl to a pattern designator that will be instantiated again.
+ExprChanged = true;
+ }
continue;
}
Index: test/SemaCXX/designated-initializers.cpp
===
--- /dev/null
+++ test/SemaCXX/designated-initializers.cpp
@@ -0,0 +1,31 @@
+// RUN: %clang_cc1 -std=c++11 -fsyntax-only -verify -Winitializer-overrides %s
+
+template struct Foo {
+ struct SubFoo {
+int bar1;
+int bar2;
+ };
+
+ static void Test() { SubFoo sf = {.bar1 = 10, .bar2 = 20}; } // Expected no warning
+};
+
+void foo() {
+ Foo::Test();
+ Foo::Test();
+ Foo::Test();
+}
+
+template struct Bar {
+ struct SubFoo {
+int bar1;
+int bar2;
+ };
+
+ static void Test() { SubFoo sf = {.bar1 = 10,// expected-note 2 {{previous initialization is here}}
+.bar1 = 20}; } // expected-warning 2 {{initializer overrides prior initialization of this subobject}}
+};
+
+void bar() {
+ Bar::Test(); // expected-note {{in instantiation of member function 'Bar::Test' requested here}}
+ Bar::Test(); // expected-note {{in instantiation of member function 'Bar::Test' requested here}}
+}
Index: lib/Sema/TreeTransform.h
===
--- lib/Sema/TreeTransform.h
+++ lib/Sema/TreeTransform.h
@@ -8923,6 +8923,19 @@
Desig.AddDesignator(Designator::getField(D.getFieldName(),
D.getDotLoc(),
D.getFieldLoc()));
+ if (D.getField()) {
+FieldDecl *Field = dyn_cast_or_null(
+getDerived().TransformDecl(D.getFieldLoc(), D.getField()));
+if (Field != D.getField())
+ // Rebuild the expression when the transformed FieldDecl is
+ // different to the already assigned FieldDecl.
+ ExprChanged = true;
+ } else {
+// Ensure that the designator expression is rebuilt when there isn't
+// a resolved FieldDecl in the designator as we don't want to assign
+// a FieldDecl to a pattern designator that will be instantiated again.
+ExprChanged = true;
+ }
continue;
}
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D25777: [Sema][TreeTransform] Re-create DesignatedInitExpr when it has a field designator with a valid FieldDecl
arphaman added a comment. In https://reviews.llvm.org/D25777#575564, @rjmccall wrote: > The fact that this bug only arises when performing a *second* instantiation > suggests that there's a deeper bug here, because template instantiation is > not supposed to modify the pattern AST. In this case, the basic problem is > that, when the parser processes a designator, it only has an identifier, not > a FieldDecl*, because it doesn't know what type is being initialized yet. > SemaInit eventually resolves that identifier to a FieldDecl and needs to > record that in the AST; typically the AST is treated as immutable, but in > this case, instead of cloning the expression, Sema just modifies the field > designator in-place. That's not completely unreasonable, and it's definitely > the most space-efficient solution for non-template code-building; but in > template code, it does mean that we have to take care to not present the same > unresolved field designator to Sema twice. > > Fortunately, this is pretty easy: we just need to need to flag the expression > as needing rebuilding when there isn't a resolved field in the field > designator. When there *is* a resolved field, we just need to map it using > TransformDecl; the expression then only needs to be rebuilt if that fails or > returns a different declaration. You're right, the point that you made about modifying the pattern AST is correct. I will update the patch accordingly. Repository: rL LLVM https://reviews.llvm.org/D25777 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D25777: [Sema][TreeTransform] Re-create DesignatedInitExpr when it has a field designator with a valid FieldDecl
rjmccall added a comment. The fact that this bug only arises when performing a *second* instantiation suggests that there's a deeper bug here, because template instantiation is not supposed to modify the pattern AST. In this case, the basic problem is that, when the parser processes a designator, it only has an identifier, not a FieldDecl*, because it doesn't know what type is being initialized yet. SemaInit eventually resolves that identifier to a FieldDecl and needs to record that in the AST; typically the AST is treated as immutable, but in this case, instead of cloning the expression, Sema just modifies the field designator in-place. That's not completely unreasonable, and it's definitely the most space-efficient solution for non-template code-building; but in template code, it does mean that we have to take care to not present the same unresolved field designator to Sema twice. Fortunately, this is pretty easy: we just need to need to flag the expression as needing rebuilding when there isn't a resolved field in the field designator. When there *is* a resolved field, we just need to map it using TransformDecl; the expression then only needs to be rebuilt if that fails or returns a different declaration. Repository: rL LLVM https://reviews.llvm.org/D25777 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D25777: [Sema][TreeTransform] Re-create DesignatedInitExpr when it has a field designator with a valid FieldDecl
arphaman updated this revision to Diff 75279.
arphaman marked an inline comment as done.
arphaman added a comment.
The updated patch adds a comment to the modified code as request by Manman.
Repository:
rL LLVM
https://reviews.llvm.org/D25777
Files:
lib/Sema/TreeTransform.h
test/SemaCXX/designated-initializers.cpp
Index: test/SemaCXX/designated-initializers.cpp
===
--- /dev/null
+++ test/SemaCXX/designated-initializers.cpp
@@ -0,0 +1,31 @@
+// RUN: %clang_cc1 -std=c++11 -fsyntax-only -verify -Winitializer-overrides %s
+
+template struct Foo {
+ struct SubFoo {
+int bar1;
+int bar2;
+ };
+
+ static void Test() { SubFoo sf = {.bar1 = 10, .bar2 = 20}; } // Expected no
warning
+};
+
+void foo() {
+ Foo::Test();
+ Foo::Test();
+ Foo::Test();
+}
+
+template struct Bar {
+ struct SubFoo {
+int bar1;
+int bar2;
+ };
+
+ static void Test() { SubFoo sf = {.bar1 = 10,// expected-note 2
{{previous initialization is here}}
+.bar1 = 20}; } // expected-warning 2
{{initializer overrides prior initialization of this subobject}}
+};
+
+void bar() {
+ Bar::Test(); // expected-note {{in instantiation of member function
'Bar::Test' requested here}}
+ Bar::Test(); // expected-note {{in instantiation of member function
'Bar::Test' requested here}}
+}
Index: lib/Sema/TreeTransform.h
===
--- lib/Sema/TreeTransform.h
+++ lib/Sema/TreeTransform.h
@@ -8923,6 +8923,12 @@
Desig.AddDesignator(Designator::getField(D.getFieldName(),
D.getDotLoc(),
D.getFieldLoc()));
+ if (D.getField())
+// Ensure that the designator expression is rebuilt when a field
+// designator was already assigned to a specific FieldDecl because
+// that FieldDecl may be incorrect in the transformed AST if it refers
+// to a FieldDecl that has been transformed as well.
+ExprChanged = true;
continue;
}
Index: test/SemaCXX/designated-initializers.cpp
===
--- /dev/null
+++ test/SemaCXX/designated-initializers.cpp
@@ -0,0 +1,31 @@
+// RUN: %clang_cc1 -std=c++11 -fsyntax-only -verify -Winitializer-overrides %s
+
+template struct Foo {
+ struct SubFoo {
+int bar1;
+int bar2;
+ };
+
+ static void Test() { SubFoo sf = {.bar1 = 10, .bar2 = 20}; } // Expected no warning
+};
+
+void foo() {
+ Foo::Test();
+ Foo::Test();
+ Foo::Test();
+}
+
+template struct Bar {
+ struct SubFoo {
+int bar1;
+int bar2;
+ };
+
+ static void Test() { SubFoo sf = {.bar1 = 10,// expected-note 2 {{previous initialization is here}}
+.bar1 = 20}; } // expected-warning 2 {{initializer overrides prior initialization of this subobject}}
+};
+
+void bar() {
+ Bar::Test(); // expected-note {{in instantiation of member function 'Bar::Test' requested here}}
+ Bar::Test(); // expected-note {{in instantiation of member function 'Bar::Test' requested here}}
+}
Index: lib/Sema/TreeTransform.h
===
--- lib/Sema/TreeTransform.h
+++ lib/Sema/TreeTransform.h
@@ -8923,6 +8923,12 @@
Desig.AddDesignator(Designator::getField(D.getFieldName(),
D.getDotLoc(),
D.getFieldLoc()));
+ if (D.getField())
+// Ensure that the designator expression is rebuilt when a field
+// designator was already assigned to a specific FieldDecl because
+// that FieldDecl may be incorrect in the transformed AST if it refers
+// to a FieldDecl that has been transformed as well.
+ExprChanged = true;
continue;
}
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D25777: [Sema][TreeTransform] Re-create DesignatedInitExpr when it has a field designator with a valid FieldDecl
manmanren added a comment. It makes sense to rebuild the expression when a field designator stores a FieldDecl. Thanks for working on this! Manman Comment at: lib/Sema/TreeTransform.h:8926 D.getFieldLoc())); + if (D.getField()) +ExprChanged = true; Please add comment here on why we need to set ExprChanged to true. Repository: rL LLVM https://reviews.llvm.org/D25777 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D25777: [Sema][TreeTransform] Re-create DesignatedInitExpr when it has a field designator with a valid FieldDecl
arphaman created this revision.
arphaman added reviewers: rjmccall, manmanren.
arphaman added a subscriber: cfe-commits.
arphaman set the repository for this revision to rL LLVM.
This patch fixes an invalid `Winitializer-overrides` warning that's shown for a
piece of code like this:
template struct Foo {
struct SubFoo {
int bar1;
int bar2;
};
static void Test() {
SubFoo sf = {.bar1 = 10,
// Incorrect "initializer overrides prior initialization" warning shown on
the line below when instantiating Foo
.bar2 = 20};
}
};
void foo() {
Foo::Test();
Foo::Test();
}
The invalid warning is shown because in the second instantiation of
`Foo::Test` the `DesignatedInitExpr` that corresponds to the initializer for
`sf` has a field designator with a pointer to the `FieldDecl` from the first
instantiation of `Foo::SubFoo`, which is incorrect in the context of the
second instantiation. This means that
`InitListChecker::CheckDesignatedInitializer` isn't able to find the correct
`FieldIndex` for the correct field in the initialized record, leading to an
incorrect warning.
This patch fixes this bug by ensuring that a `DesignatedInitExpr` is re-created
by the tree-transformer when it has a field designator with a `FieldDecl` that
has been already been set. This means that in the example above the
`DesignatedInitExpr` won't be re-created by the tree-transformer in the first
instantiation, but it will be re-created in the second instantiation, thus
ensuring that the second instantiation doesn't have the incorrect `FieldDecl`
pointers.
Repository:
rL LLVM
https://reviews.llvm.org/D25777
Files:
lib/Sema/TreeTransform.h
test/SemaCXX/designated-initializers.cpp
Index: test/SemaCXX/designated-initializers.cpp
===
--- /dev/null
+++ test/SemaCXX/designated-initializers.cpp
@@ -0,0 +1,31 @@
+// RUN: %clang_cc1 -std=c++11 -fsyntax-only -verify -Winitializer-overrides %s
+
+template struct Foo {
+ struct SubFoo {
+int bar1;
+int bar2;
+ };
+
+ static void Test() { SubFoo sf = {.bar1 = 10, .bar2 = 20}; } // Expected no
warning
+};
+
+void foo() {
+ Foo::Test();
+ Foo::Test();
+ Foo::Test();
+}
+
+template struct Bar {
+ struct SubFoo {
+int bar1;
+int bar2;
+ };
+
+ static void Test() { SubFoo sf = {.bar1 = 10,// expected-note 2
{{previous initialization is here}}
+.bar1 = 20}; } // expected-warning 2
{{initializer overrides prior initialization of this subobject}}
+};
+
+void bar() {
+ Bar::Test(); // expected-note {{in instantiation of member function
'Bar::Test' requested here}}
+ Bar::Test(); // expected-note {{in instantiation of member function
'Bar::Test' requested here}}
+}
Index: lib/Sema/TreeTransform.h
===
--- lib/Sema/TreeTransform.h
+++ lib/Sema/TreeTransform.h
@@ -8923,6 +8923,8 @@
Desig.AddDesignator(Designator::getField(D.getFieldName(),
D.getDotLoc(),
D.getFieldLoc()));
+ if (D.getField())
+ExprChanged = true;
continue;
}
Index: test/SemaCXX/designated-initializers.cpp
===
--- /dev/null
+++ test/SemaCXX/designated-initializers.cpp
@@ -0,0 +1,31 @@
+// RUN: %clang_cc1 -std=c++11 -fsyntax-only -verify -Winitializer-overrides %s
+
+template struct Foo {
+ struct SubFoo {
+int bar1;
+int bar2;
+ };
+
+ static void Test() { SubFoo sf = {.bar1 = 10, .bar2 = 20}; } // Expected no warning
+};
+
+void foo() {
+ Foo::Test();
+ Foo::Test();
+ Foo::Test();
+}
+
+template struct Bar {
+ struct SubFoo {
+int bar1;
+int bar2;
+ };
+
+ static void Test() { SubFoo sf = {.bar1 = 10,// expected-note 2 {{previous initialization is here}}
+.bar1 = 20}; } // expected-warning 2 {{initializer overrides prior initialization of this subobject}}
+};
+
+void bar() {
+ Bar::Test(); // expected-note {{in instantiation of member function 'Bar::Test' requested here}}
+ Bar::Test(); // expected-note {{in instantiation of member function 'Bar::Test' requested here}}
+}
Index: lib/Sema/TreeTransform.h
===
--- lib/Sema/TreeTransform.h
+++ lib/Sema/TreeTransform.h
@@ -8923,6 +8923,8 @@
Desig.AddDesignator(Designator::getField(D.getFieldName(),
D.getDotLoc(),
D.getFieldLoc()));
+ if (D.getField())
+ExprChanged = true;
continue;
}
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
