[clang][AST] Remove HasFirstArg assertion in CallExpr::getBeginLoc() #130725
Conversation
|
@llvm/pr-subscribers-clang Author: Nathan Ridge (HighCommander4) ChangesThere are cases where the assertion legitimately does not hold (e.g. CallExpr::CreateTemporary()), and there's no readily available way to tell such cases apart. Fixes #130272 Full diff: https://github.com/llvm/llvm-project/pull/130725.diff 2 Files Affected:
diff --git a/clang/lib/AST/Expr.cpp b/clang/lib/AST/Expr.cpp
index ccfec7fda0cbc..1dde64f193dbd 100644
--- a/clang/lib/AST/Expr.cpp
+++ b/clang/lib/AST/Expr.cpp
@@ -1656,9 +1656,11 @@ SourceLocation CallExpr::getBeginLoc() const {
if (const auto *Method =
dyn_cast_if_present<const CXXMethodDecl>(getCalleeDecl());
Method && Method->isExplicitObjectMemberFunction()) {
- bool HasFirstArg = getNumArgs() > 0 && getArg(0);
- assert(HasFirstArg);
- if (HasFirstArg)
+ // Note: while we typically expect the call to have a first argument
+ // here, we can't assert it because in some cases it does not, e.g.
+ // calls created with CallExpr::CreateTemporary() during overload
+ // resolution.
+ if (getNumArgs() > 0 && getArg(0))
return getArg(0)->getBeginLoc();
}
}
diff --git a/clang/test/AST/ast-dump-cxx2b-deducing-this.cpp b/clang/test/AST/ast-dump-cxx2b-deducing-this.cpp
index abe9d6a5b5bc6..fc86aeb3e5ec3 100644
--- a/clang/test/AST/ast-dump-cxx2b-deducing-this.cpp
+++ b/clang/test/AST/ast-dump-cxx2b-deducing-this.cpp
@@ -26,3 +26,12 @@ struct S {
// CHECK-NEXT: `-DeclRefExpr 0x{{[^ ]*}} <col:5> 'S<T>' lvalue ParmVar 0x{{[^ ]*}} 's' 'S<T>'
};
}
+
+namespace GH130272 {
+struct A {};
+struct B {
+ operator A(this B);
+};
+A a = A(B{});
+// CHECK: CallExpr 0x{{[^ ]*}} <col:9, col:11> 'A':'GH130272::A'
+}
|
|
@cor3ntin I would appreciate it if you can take a look too |
There was a problem hiding this comment.
Missing changelog.
Otherwise, I think it makes sense even if I sort of hate it, as it relies on TryCopyInitialization looking at the location and nothing else.
I wonder if we could get rid of CallExpr::CreateTemporary and instead
create something like an OpaqueValueExpr for the purpose of determining an initialization sequence.
(We should only need some expression with a type, a location, and a value category, afaik)
There are cases where the assertion legitimately does not hold (e.g. CallExpr::CreateTemporary()), and there's no readily available way to tell such cases apart. Fixes llvm#130272
6a4f6cf to
d6c4f88
Compare
Added.
Filed #130824 to track this idea. |
`CallExpr::CreateTemporary` was only used to deduce a conversion sequence from a conversion operator. We only need a type/value category for that, so we can use a dummy Expression such as a `OpaqueValueExpr`. This simplify the code and avoid partially-formed `CallExpr` with incorrect invariants (see llvm#130725) Fixes llvm#130824
`CallExpr::CreateTemporary` was only used to deduce a conversion sequence from a conversion operator. We only need a type/value category for that, so we can use a dummy Expression such as a `OpaqueValueExpr`. This simplify the code and avoid partially-formed `CallExpr` with incorrect invariants (see #130725) Fixes #130824
There are cases where the assertion legitimately does not hold (e.g. CallExpr::CreateTemporary()), and there's no readily available way to tell such cases apart.
Fixes #130272