[PATCH] D18425: [Sema] Make enable_if act correctly with value dependent conditions/arguments

[George Burgess IV via cfe-commits]

george.burgess.iv added inline comments.

================
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);
 
----------------
rsmith wrote:
> 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).
SG; will just revert.

================
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))
----------------
rsmith wrote:
> Can you delete this FIXME? I'm pretty sure we've got that right for years :)
r265341

================
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);
+  }
+
----------------
rsmith wrote:
> 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.)
If instantiation dependence is enough to be sure that we re-evaluate the `enable_if` condition(s) when we have all of the values we need, then not marking this as value-dependent seems like a better approach, yeah. :)

> 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

Given that we no longer care about value-dependence, it doesn't matter, but will this also happen when the `enable_if` condition itself is value-dependent?

================
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;
----------------
rsmith wrote:
> 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.
Works for me; will come bug you soonish.


http://reviews.llvm.org/D18425