[PATCH] D58659: [Sema] Fix assertion when `auto` parameter in lambda has an attribute.
This revision was automatically updated to reflect the committed changes.
Closed by commit rL357298: [Sema] Fix assertion when `auto` parameter in lambda
has an attribute. (authored by vsapsai, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.
Changed prior to commit:
https://reviews.llvm.org/D58659?vs=188283&id=192878#toc
Repository:
rL LLVM
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D58659/new/
https://reviews.llvm.org/D58659
Files:
cfe/trunk/lib/Sema/SemaType.cpp
cfe/trunk/test/SemaCXX/auto-cxx0x.cpp
Index: cfe/trunk/lib/Sema/SemaType.cpp
===
--- cfe/trunk/lib/Sema/SemaType.cpp
+++ cfe/trunk/lib/Sema/SemaType.cpp
@@ -255,6 +255,23 @@
return T;
}
+/// Completely replace the \c auto in \p TypeWithAuto by
+/// \p Replacement. Also replace \p TypeWithAuto in \c TypeAttrPair if
+/// necessary.
+QualType ReplaceAutoType(QualType TypeWithAuto, QualType Replacement) {
+ QualType T = sema.ReplaceAutoType(TypeWithAuto, Replacement);
+ if (auto *AttrTy = TypeWithAuto->getAs()) {
+// Attributed type still should be an attributed type after
replacement.
+auto *NewAttrTy = cast(T.getTypePtr());
+for (TypeAttrPair &A : AttrsForTypes) {
+ if (A.first == AttrTy)
+A.first = NewAttrTy;
+}
+AttrsForTypesSorted = false;
+ }
+ return T;
+}
+
/// Extract and remove the Attr* for a given attributed type.
const Attr *takeAttrForAttributedType(const AttributedType *AT) {
if (!AttrsForTypesSorted) {
@@ -2938,7 +2955,7 @@
// template type parameter.
// FIXME: Retain some type sugar to indicate that this was written
// as 'auto'.
-T = SemaRef.ReplaceAutoType(
+T = state.ReplaceAutoType(
T, QualType(CorrespondingTemplateParam->getTypeForDecl(), 0));
}
break;
Index: cfe/trunk/test/SemaCXX/auto-cxx0x.cpp
===
--- cfe/trunk/test/SemaCXX/auto-cxx0x.cpp
+++ cfe/trunk/test/SemaCXX/auto-cxx0x.cpp
@@ -15,3 +15,11 @@
// expected-error@-2 {{'auto' not allowed in lambda parameter}}
#endif
}
+
+void rdar47689465() {
+ int x = 0;
+ [](auto __attribute__((noderef)) *){}(&x);
+#if __cplusplus == 201103L
+ // expected-error@-2 {{'auto' not allowed in lambda parameter}}
+#endif
+}
Index: cfe/trunk/lib/Sema/SemaType.cpp
===
--- cfe/trunk/lib/Sema/SemaType.cpp
+++ cfe/trunk/lib/Sema/SemaType.cpp
@@ -255,6 +255,23 @@
return T;
}
+/// Completely replace the \c auto in \p TypeWithAuto by
+/// \p Replacement. Also replace \p TypeWithAuto in \c TypeAttrPair if
+/// necessary.
+QualType ReplaceAutoType(QualType TypeWithAuto, QualType Replacement) {
+ QualType T = sema.ReplaceAutoType(TypeWithAuto, Replacement);
+ if (auto *AttrTy = TypeWithAuto->getAs()) {
+// Attributed type still should be an attributed type after replacement.
+auto *NewAttrTy = cast(T.getTypePtr());
+for (TypeAttrPair &A : AttrsForTypes) {
+ if (A.first == AttrTy)
+A.first = NewAttrTy;
+}
+AttrsForTypesSorted = false;
+ }
+ return T;
+}
+
/// Extract and remove the Attr* for a given attributed type.
const Attr *takeAttrForAttributedType(const AttributedType *AT) {
if (!AttrsForTypesSorted) {
@@ -2938,7 +2955,7 @@
// template type parameter.
// FIXME: Retain some type sugar to indicate that this was written
// as 'auto'.
-T = SemaRef.ReplaceAutoType(
+T = state.ReplaceAutoType(
T, QualType(CorrespondingTemplateParam->getTypeForDecl(), 0));
}
break;
Index: cfe/trunk/test/SemaCXX/auto-cxx0x.cpp
===
--- cfe/trunk/test/SemaCXX/auto-cxx0x.cpp
+++ cfe/trunk/test/SemaCXX/auto-cxx0x.cpp
@@ -15,3 +15,11 @@
// expected-error@-2 {{'auto' not allowed in lambda parameter}}
#endif
}
+
+void rdar47689465() {
+ int x = 0;
+ [](auto __attribute__((noderef)) *){}(&x);
+#if __cplusplus == 201103L
+ // expected-error@-2 {{'auto' not allowed in lambda parameter}}
+#endif
+}
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D58659: [Sema] Fix assertion when `auto` parameter in lambda has an attribute.
vsapsai added a comment. Thanks for the review, Aaron. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D58659/new/ https://reviews.llvm.org/D58659 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D58659: [Sema] Fix assertion when `auto` parameter in lambda has an attribute.
aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.
LGTM!
Comment at: clang/lib/Sema/SemaType.cpp:261
+/// necessary.
+QualType ReplaceAutoType(QualType TypeWithAuto, QualType Replacement) {
+ QualType T = sema.ReplaceAutoType(TypeWithAuto, Replacement);
vsapsai wrote:
> aaron.ballman wrote:
> > I think this work should be done within `SubstituteDeducedTypeTransform`
> > rather than on the side. Any caller to `Sema::ReplaceAutoType()` should get
> > this same behavior.
> Doing this work in `SubstituteDeducedTypeTransform` involves teaching
> `SubstituteDeducedTypeTransform` about `TypeProcessingState` and probably
> adding `TypeProcessingState` to `Sema::ReplaceAutoType()`. As for me, it
> exposes `TypeProcessingState` in more places than necessary. And it feels
> somewhat awkward that `SubstituteDeducedTypeTransform` is used in multiple
> places but `TypeProcessingState` is required only here.
>
> I've modelled my approach after `TypeProcessingState::getAttributedType`
> where it forwards the call to Sema and keeps `AttrsForTypes` up to date. Do
> you still think `SubstituteDeducedTypeTransform` would be a better place for
> this code?
Hmm, yeah, it doesn't seem like it would make sense there. This is keeping the
`TypeProcessingState` up to date which is really only of interest during some
stages of processing.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D58659/new/
https://reviews.llvm.org/D58659
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D58659: [Sema] Fix assertion when `auto` parameter in lambda has an attribute.
vsapsai added inline comments.
Comment at: clang/lib/Sema/SemaType.cpp:261
+/// necessary.
+QualType ReplaceAutoType(QualType TypeWithAuto, QualType Replacement) {
+ QualType T = sema.ReplaceAutoType(TypeWithAuto, Replacement);
aaron.ballman wrote:
> I think this work should be done within `SubstituteDeducedTypeTransform`
> rather than on the side. Any caller to `Sema::ReplaceAutoType()` should get
> this same behavior.
Doing this work in `SubstituteDeducedTypeTransform` involves teaching
`SubstituteDeducedTypeTransform` about `TypeProcessingState` and probably
adding `TypeProcessingState` to `Sema::ReplaceAutoType()`. As for me, it
exposes `TypeProcessingState` in more places than necessary. And it feels
somewhat awkward that `SubstituteDeducedTypeTransform` is used in multiple
places but `TypeProcessingState` is required only here.
I've modelled my approach after `TypeProcessingState::getAttributedType` where
it forwards the call to Sema and keeps `AttrsForTypes` up to date. Do you still
think `SubstituteDeducedTypeTransform` would be a better place for this code?
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D58659/new/
https://reviews.llvm.org/D58659
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D58659: [Sema] Fix assertion when `auto` parameter in lambda has an attribute.
aaron.ballman added inline comments.
Comment at: clang/lib/Sema/SemaType.cpp:261
+/// necessary.
+QualType ReplaceAutoType(QualType TypeWithAuto, QualType Replacement) {
+ QualType T = sema.ReplaceAutoType(TypeWithAuto, Replacement);
I think this work should be done within `SubstituteDeducedTypeTransform` rather
than on the side. Any caller to `Sema::ReplaceAutoType()` should get this same
behavior.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D58659/new/
https://reviews.llvm.org/D58659
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D58659: [Sema] Fix assertion when `auto` parameter in lambda has an attribute.
vsapsai added inline comments.
Comment at: clang/lib/Sema/SemaType.cpp:266
+auto *NewAttrTy = cast(T.getTypePtr());
+for (TypeAttrPair &A : AttrsForTypes) {
+ if (A.first == AttrTy)
jkorous wrote:
> Do you think it would make sense to terminate the loop early if
> `AttrsForTypesSorted == true`?
I agree with you it would be nice to reuse `AttrsForTypesSorted` property but
in practice it doesn't matter. We sort `AttrsForTypes` only in
`takeAttrForAttributedType` and we should do that after we've populated
`AttrsForTypes` and after we've replaced `auto` types. It isn't strictly
required but that's how it was working in debugger. Also I've tried to add
`assert(! AttrsForTypesSorted);` and it wasn't triggered during `ninja
check-clang`
Having `AttrsForTypes` sorted can be useful in the future but I don't think we
should add code for that now.
Despite the linear iteration, we are not pessimizing the most common use case
with no attributes for lambda parameters (expression "most common use case" is
based on my experience and not substantiated by any data). So I think the
performance shouldn't degrade noticeably.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D58659/new/
https://reviews.llvm.org/D58659
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D58659: [Sema] Fix assertion when `auto` parameter in lambda has an attribute.
jkorous added inline comments.
Comment at: clang/lib/Sema/SemaType.cpp:266
+auto *NewAttrTy = cast(T.getTypePtr());
+for (TypeAttrPair &A : AttrsForTypes) {
+ if (A.first == AttrTy)
Do you think it would make sense to terminate the loop early if
`AttrsForTypesSorted == true`?
Comment at: clang/lib/Sema/SemaType.cpp:270
+}
+AttrsForTypesSorted = false;
+ }
Do you think it would be reasonable to conditionally assign (or rather keep)
`AttrsForTypesSorted = true` in case they were sorted previously and new key
doesn't break the order?
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D58659/new/
https://reviews.llvm.org/D58659
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D58659: [Sema] Fix assertion when `auto` parameter in lambda has an attribute.
vsapsai added a comment. Have doubts that checking only the outermost type for being `AttributedType` after `Sema::ReplaceAutoType` is sufficient but haven't found a counterexample yet. Decided to post the proposed fix to see if others have any ideas. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D58659/new/ https://reviews.llvm.org/D58659 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D58659: [Sema] Fix assertion when `auto` parameter in lambda has an attribute.
vsapsai created this revision.
vsapsai added reviewers: rsmith, arphaman.
Herald added subscribers: jdoerfert, dexonsmith, jkorous.
Fixes the assertion
> no Attr* for AttributedType*
> UNREACHABLE executed at llvm-project/clang/lib/Sema/SemaType.cpp:298!
In `TypeProcessingState::getAttributedType` we put into `AttrsForTypes`
types with `auto` but later in
`TypeProcessingState::takeAttrForAttributedType` we use transformed
types and that's why cannot find `Attr` corresponding to
`AttributedType`.
Fix by keeping `AttrsForTypes` up to date after replacing `AutoType`.
rdar://problem/47689465
https://reviews.llvm.org/D58659
Files:
clang/lib/Sema/SemaType.cpp
clang/test/SemaCXX/auto-cxx0x.cpp
Index: clang/test/SemaCXX/auto-cxx0x.cpp
===
--- clang/test/SemaCXX/auto-cxx0x.cpp
+++ clang/test/SemaCXX/auto-cxx0x.cpp
@@ -15,3 +15,11 @@
// expected-error@-2 {{'auto' not allowed in lambda parameter}}
#endif
}
+
+void rdar47689465() {
+ int x = 0;
+ [](auto __attribute__((noderef)) *){}(&x);
+#if __cplusplus == 201103L
+ // expected-error@-2 {{'auto' not allowed in lambda parameter}}
+#endif
+}
Index: clang/lib/Sema/SemaType.cpp
===
--- clang/lib/Sema/SemaType.cpp
+++ clang/lib/Sema/SemaType.cpp
@@ -255,6 +255,23 @@
return T;
}
+/// Completely replace the \c auto in \p TypeWithAuto by
+/// \p Replacement. Also replace \p TypeWithAuto in \c TypeAttrPair if
+/// necessary.
+QualType ReplaceAutoType(QualType TypeWithAuto, QualType Replacement) {
+ QualType T = sema.ReplaceAutoType(TypeWithAuto, Replacement);
+ if (auto *AttrTy = TypeWithAuto->getAs()) {
+// Attributed type still should be an attributed type after
replacement.
+auto *NewAttrTy = cast(T.getTypePtr());
+for (TypeAttrPair &A : AttrsForTypes) {
+ if (A.first == AttrTy)
+A.first = NewAttrTy;
+}
+AttrsForTypesSorted = false;
+ }
+ return T;
+}
+
/// Extract and remove the Attr* for a given attributed type.
const Attr *takeAttrForAttributedType(const AttributedType *AT) {
if (!AttrsForTypesSorted) {
@@ -2935,7 +2952,7 @@
// template type parameter.
// FIXME: Retain some type sugar to indicate that this was written
// as 'auto'.
-T = SemaRef.ReplaceAutoType(
+T = state.ReplaceAutoType(
T, QualType(CorrespondingTemplateParam->getTypeForDecl(), 0));
}
break;
Index: clang/test/SemaCXX/auto-cxx0x.cpp
===
--- clang/test/SemaCXX/auto-cxx0x.cpp
+++ clang/test/SemaCXX/auto-cxx0x.cpp
@@ -15,3 +15,11 @@
// expected-error@-2 {{'auto' not allowed in lambda parameter}}
#endif
}
+
+void rdar47689465() {
+ int x = 0;
+ [](auto __attribute__((noderef)) *){}(&x);
+#if __cplusplus == 201103L
+ // expected-error@-2 {{'auto' not allowed in lambda parameter}}
+#endif
+}
Index: clang/lib/Sema/SemaType.cpp
===
--- clang/lib/Sema/SemaType.cpp
+++ clang/lib/Sema/SemaType.cpp
@@ -255,6 +255,23 @@
return T;
}
+/// Completely replace the \c auto in \p TypeWithAuto by
+/// \p Replacement. Also replace \p TypeWithAuto in \c TypeAttrPair if
+/// necessary.
+QualType ReplaceAutoType(QualType TypeWithAuto, QualType Replacement) {
+ QualType T = sema.ReplaceAutoType(TypeWithAuto, Replacement);
+ if (auto *AttrTy = TypeWithAuto->getAs()) {
+// Attributed type still should be an attributed type after replacement.
+auto *NewAttrTy = cast(T.getTypePtr());
+for (TypeAttrPair &A : AttrsForTypes) {
+ if (A.first == AttrTy)
+A.first = NewAttrTy;
+}
+AttrsForTypesSorted = false;
+ }
+ return T;
+}
+
/// Extract and remove the Attr* for a given attributed type.
const Attr *takeAttrForAttributedType(const AttributedType *AT) {
if (!AttrsForTypesSorted) {
@@ -2935,7 +2952,7 @@
// template type parameter.
// FIXME: Retain some type sugar to indicate that this was written
// as 'auto'.
-T = SemaRef.ReplaceAutoType(
+T = state.ReplaceAutoType(
T, QualType(CorrespondingTemplateParam->getTypeForDecl(), 0));
}
break;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
