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

[Richard Smith via cfe-commits]

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