rsmith added inline comments.
================
Comment at: include/clang/Sema/Sema.h:2489
@@ -2485,1 +2488,3 @@
+ bool MissingImplicitThis = false,
+ bool *FailedDueToValueDependentExpr = nullptr);
----------------
I'd just call this `Dependent` -- that is, "here is a dependent `enable_if`
attribute, so I couldn't tell whether this is enabled". The details of why it's
dependent shouldn't be relevant to the caller.
================
Comment at: lib/Sema/SemaExpr.cpp:5047-5098
@@ -5046,57 +5046,54 @@
+CallExpr *Sema::buildDependentCallExpr(Expr *ExecConfig, Expr *Fn,
+ MultiExprArg ArgExprs,
+ SourceLocation RParenLoc) {
+ if (ExecConfig)
+ return new (Context)
+ CUDAKernelCallExpr(Context, Fn, cast<CallExpr>(ExecConfig), ArgExprs,
+ Context.DependentTy, VK_RValue, RParenLoc);
+ return new (Context) CallExpr(Context, Fn, ArgExprs, Context.DependentTy,
+ VK_RValue, RParenLoc);
+}
+
/// ActOnCallExpr - Handle a call to Fn with the specified array of arguments.
/// This provides the location of the left/right parens and a list of comma
/// locations.
ExprResult
Sema::ActOnCallExpr(Scope *S, Expr *Fn, SourceLocation LParenLoc,
MultiExprArg ArgExprs, SourceLocation RParenLoc,
Expr *ExecConfig, bool IsExecConfig) {
// Since this might be a postfix expression, get rid of ParenListExprs.
ExprResult Result = MaybeConvertParenListExprToParenExpr(S, Fn);
if (Result.isInvalid()) return ExprError();
Fn = Result.get();
if (checkArgsForPlaceholders(*this, ArgExprs))
return ExprError();
if (getLangOpts().CPlusPlus) {
// If this is a pseudo-destructor expression, build the call immediately.
if (isa<CXXPseudoDestructorExpr>(Fn)) {
if (!ArgExprs.empty()) {
// Pseudo-destructor calls should not have any arguments.
Diag(Fn->getLocStart(), diag::err_pseudo_dtor_call_with_args)
<< FixItHint::CreateRemoval(
SourceRange(ArgExprs.front()->getLocStart(),
ArgExprs.back()->getLocEnd()));
}
return new (Context)
CallExpr(Context, Fn, None, Context.VoidTy, VK_RValue, RParenLoc);
}
if (Fn->getType() == Context.PseudoObjectTy) {
ExprResult result = CheckPlaceholderExpr(Fn);
if (result.isInvalid()) return ExprError();
Fn = result.get();
}
// Determine whether this is a dependent call inside a C++ template,
// in which case we won't do any semantic analysis now.
// FIXME: Will need to cache the results of name lookup (including ADL) in
// Fn.
- bool Dependent = false;
- if (Fn->isTypeDependent())
- Dependent = true;
- else if (Expr::hasAnyTypeDependentArguments(ArgExprs))
- Dependent = true;
-
- if (Dependent) {
- if (ExecConfig) {
- return new (Context) CUDAKernelCallExpr(
- Context, Fn, cast<CallExpr>(ExecConfig), ArgExprs,
- Context.DependentTy, VK_RValue, RParenLoc);
- } else {
- return new (Context) CallExpr(
- Context, Fn, ArgExprs, Context.DependentTy, VK_RValue, RParenLoc);
- }
- }
+ if (Fn->isTypeDependent() || Expr::hasAnyTypeDependentArguments(ArgExprs))
+ return buildDependentCallExpr(ExecConfig, Fn, ArgExprs, RParenLoc);
----------------
You don't seem to make use of this refactoring in the functional change below.
Either revert or commit separately, please (and if you commit it, fix the
comment in Sema.h to reflect what this is used for).
================
Comment at: lib/Sema/SemaExpr.cpp:5095-5096
@@ -5083,3 +5094,4 @@
// in which case we won't do any semantic analysis now.
// FIXME: Will need to cache the results of name lookup (including ADL) in
// Fn.
+ if (Fn->isTypeDependent() || Expr::hasAnyTypeDependentArguments(ArgExprs))
----------------
Can you delete this FIXME? I'm pretty sure we've got that right for years :)
================
Comment at: lib/Sema/SemaExpr.cpp:5202-5205
@@ +5201,6 @@
+ Fn, NDecl, LParenLoc, ArgExprs, RParenLoc, ExecConfig, IsExecConfig);
+ if (MakeCallValueDependent) {
+ BuiltCall.get()->setValueDependent(true);
+ BuiltCall.get()->setInstantiationDependent(true);
+ }
+
----------------
Does this need to be value-dependent? The only possible outcomes here are
either that we call the designated function or that instantiation fails, which
seems like instantiation-dependence is enough. (In practice, the
`setValueDependent` call will be a no-op, because the only way the flag gets
set is if one of the `ArgExprs` is value-dependent, which results in the call
being value-dependent regardless, so there should be no functional change in
removing the `setValueDependent` call.)
================
Comment at: lib/Sema/SemaOverload.cpp:5872
@@ -5845,2 +5871,3 @@
+ if (void *Failed = packedCheckEnableIf(*this, Function, Args)) {
Candidate.Viable = false;
Candidate.FailureKind = ovl_fail_enable_if;
----------------
I'm nervous about setting `Viable` to `false` for a function that *might be*
viable; I expect a lot of code to "misinterpret" that. If we go ahead with this
direction, I'd prefer for the `Viable` flag to become an enum of `NonViable`,
`Viable`, `Dependent`, have overload resolution treat dependently-viable
functions like viable functions, and add an `OverloadingResult` of
`OR_Dependent` for the case where there was a unique best result that was
dependently viable. All 31 callers of `BestViableFunction` would then need to
check for and handle the new case, and build a dependent call expression in
that case.
However, this seems like a lot of complexity to deal with this case. Let's talk
offline and see if we can come up with something better.
http://reviews.llvm.org/D18425
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
