Prefer functions with true enable_if conditions when attempting to resolve the address of an overload

[george.burgess.iv]

Summary

Our overload resolution logic considers some candidate A better than some candidate B if the set of A's enable_if conditions is a strict superset of the set of B's enable_if conditions.

Our address-of overload resolution logic does not.

This is suboptimal.

This patch updates the address-of overload resolution logic to behave more like the regular overload resolution logic.

[rsmith]

Looks great, thanks!

lib/Sema/SemaOverload.cpp

8413–8414 ↗

(On Diff #37545)

... and Cand1 has more than N enable_if attributes.

cfe/trunk/lib/Sema/SemaOverload.cpp

for (unsigned b = 0, e = Methods.size(); b < e; b++) {
     }
 
     if (Match)
       return Method;
   }
   return nullptr;
 }
 
-static bool IsNotEnableIfAttr(Attr *A) { return !isa<EnableIfAttr>(A); }
+// specific_attr_iterator iterates over enable_if attributes in reverse, and
+// enable_if is order-sensitive. As a result, we need to reverse things
+// sometimes. Size of 4 elements is arbitrary.
+static SmallVector<EnableIfAttr *, 4>
+getOrderedEnableIfAttrs(const FunctionDecl *Function) {
+  SmallVector<EnableIfAttr *, 4> Result;
+  if (!Function->hasAttrs())
+    return Result;
+
+  const auto &FuncAttrs = Function->getAttrs();
+  for (Attr *Attr : FuncAttrs)
+    if (auto *EnableIf = dyn_cast<EnableIfAttr>(Attr))
+      Result.push_back(EnableIf);
+
+  std::reverse(Result.begin(), Result.end());
+  return Result;
+}
 
 EnableIfAttr *Sema::CheckEnableIf(FunctionDecl *Function, ArrayRef<Expr *> Args,
                                   bool MissingImplicitThis) {
-  // FIXME: specific_attr_iterator<EnableIfAttr> iterates in reverse order, but
-  // we need to find the first failing one.
-  if (!Function->hasAttrs())
-    return nullptr;
-  AttrVec Attrs = Function->getAttrs();
-  AttrVec::iterator E = std::remove_if(Attrs.begin(), Attrs.end(),
-                                       IsNotEnableIfAttr);
-  if (Attrs.begin() == E)
+  auto EnableIfAttrs = getOrderedEnableIfAttrs(Function);
+  if (EnableIfAttrs.empty())
     return nullptr;
-  std::reverse(Attrs.begin(), E);
 
   SFINAETrap Trap(*this);
-
   SmallVector<Expr *, 16> ConvertedArgs;
   bool InitializationFailed = false;
   bool ContainsValueDependentExpr = false;
 
   // Convert the arguments.
   for (unsigned i = 0, e = Args.size(); i != e; ++i) {
     if (i == 0 && !MissingImplicitThis && isa<CXXMethodDecl>(Function) &&
         !cast<CXXMethodDecl>(Function)->isStatic() &&
if (i == 0 && !MissingImplicitThis && isa<CXXMethodDecl>(Function) &&
         break;
       }
       ContainsValueDependentExpr |= R.get()->isValueDependent();
       ConvertedArgs.push_back(R.get());
     }
   }
 
   if (InitializationFailed || Trap.hasErrorOccurred())
-    return cast<EnableIfAttr>(Attrs[0]);
+    return EnableIfAttrs[0];
 
   // Push default arguments if needed.
   if (!Function->isVariadic() && Args.size() < Function->getNumParams()) {
     for (unsigned i = Args.size(), e = Function->getNumParams(); i != e; ++i) {
       ParmVarDecl *P = Function->getParamDecl(i);
       ExprResult R = PerformCopyInitialization(
           InitializedEntity::InitializeParameter(Context,
                                                  Function->getParamDecl(i)),
           SourceLocation(),
           P->hasUninstantiatedDefaultArg() ? P->getUninstantiatedDefaultArg()
                                            : P->getDefaultArg());
       if (R.isInvalid()) {
         InitializationFailed = true;
         break;
       }
       ContainsValueDependentExpr |= R.get()->isValueDependent();
       ConvertedArgs.push_back(R.get());
     }
 
     if (InitializationFailed || Trap.hasErrorOccurred())
-      return cast<EnableIfAttr>(Attrs[0]);
+      return EnableIfAttrs[0];
   }
 
-  for (AttrVec::iterator I = Attrs.begin(); I != E; ++I) {
+  for (auto *EIA : EnableIfAttrs) {
     APValue Result;
-    EnableIfAttr *EIA = cast<EnableIfAttr>(*I);
     if (EIA->getCond()->isValueDependent()) {
       // Don't even try now, we'll examine it after instantiation.
       continue;
     }
 
     if (!EIA->getCond()->EvaluateWithSubstitution(
             Result, Context, Function, llvm::makeArrayRef(ConvertedArgs))) {
       if (!ContainsValueDependentExpr)
if (FunctionDecl *FD = dyn_cast<FunctionDecl>(*I)) {
                            PartialOverloading);
     } else
       AddTemplateOverloadCandidate(cast<FunctionTemplateDecl>(*I),
                                    FoundDecl, ExplicitTemplateArgs,
                                    Args, CandidateSet, PartialOverloading);
   }
 }
 
+// Determines whether Cand1 is "better" in terms of its enable_if attrs than
+// Cand2 for overloading. This function assumes that all of the enable_if attrs
+// on Cand1 and Cand2 have conditions that evaluate to true.
+//
+// Cand1's set of enable_if attributes are said to be "better" than Cand2's iff
+// Cand1's first N enable_if attributes have precisely the same conditions as
+// Cand2's first N enable_if attributes (where N = the number of enable_if
+// attributes on Cand2), and Cand1 has more than N enable_if attributes.
+static bool hasBetterEnableIfAttrs(Sema &S, const FunctionDecl *Cand1,
+                                   const FunctionDecl *Cand2) {
+
+  // FIXME: The next several lines are just
+  // specific_attr_iterator<EnableIfAttr> but going in declaration order,
+  // instead of reverse order which is how they're stored in the AST.
+  auto Cand1Attrs = getOrderedEnableIfAttrs(Cand1);
+  auto Cand2Attrs = getOrderedEnableIfAttrs(Cand2);
+
+  // Candidate 1 is better if it has strictly more attributes and
+  // the common sequence is identical.
+  if (Cand1Attrs.size() <= Cand2Attrs.size())
+    return false;
+
+  auto Cand1I = Cand1Attrs.begin();
+  llvm::FoldingSetNodeID Cand1ID, Cand2ID;
+  for (auto &Cand2A : Cand2Attrs) {
+    Cand1ID.clear();
+    Cand2ID.clear();
+
+    auto &Cand1A = *Cand1I++;
+    Cand1A->getCond()->Profile(Cand1ID, S.getASTContext(), true);
+    Cand2A->getCond()->Profile(Cand2ID, S.getASTContext(), true);
+    if (Cand1ID != Cand2ID)
+      return false;
+  }
+
+  return true;
+}
+
 /// isBetterOverloadCandidate - Determines whether the first overload
 /// candidate is a better candidate than the second (C++ 13.3.3p1).
 bool clang::isBetterOverloadCandidate(Sema &S, const OverloadCandidate &Cand1,
                                       const OverloadCandidate &Cand2,
                                       SourceLocation Loc,
                                       bool UserDefinedConversion) {
   // Define viable functions to be better candidates than non-viable
   // functions.
if (FunctionTemplateDecl *BetterTemplate
                                          Cand1.ExplicitCallArguments,
                                          Cand2.ExplicitCallArguments))
       return BetterTemplate == Cand1.Function->getPrimaryTemplate();
   }
 
   // Check for enable_if value-based overload resolution.
   if (Cand1.Function && Cand2.Function &&
       (Cand1.Function->hasAttr<EnableIfAttr>() ||
-       Cand2.Function->hasAttr<EnableIfAttr>())) {
-    // FIXME: The next several lines are just
-    // specific_attr_iterator<EnableIfAttr> but going in declaration order,
-    // instead of reverse order which is how they're stored in the AST.
-    AttrVec Cand1Attrs;
-    if (Cand1.Function->hasAttrs()) {
-      Cand1Attrs = Cand1.Function->getAttrs();
-      Cand1Attrs.erase(std::remove_if(Cand1Attrs.begin(), Cand1Attrs.end(),
-                                      IsNotEnableIfAttr),
-                       Cand1Attrs.end());
-      std::reverse(Cand1Attrs.begin(), Cand1Attrs.end());
-    }
-
-    AttrVec Cand2Attrs;
-    if (Cand2.Function->hasAttrs()) {
-      Cand2Attrs = Cand2.Function->getAttrs();
-      Cand2Attrs.erase(std::remove_if(Cand2Attrs.begin(), Cand2Attrs.end(),
-                                      IsNotEnableIfAttr),
-                       Cand2Attrs.end());
-      std::reverse(Cand2Attrs.begin(), Cand2Attrs.end());
-    }
-
-    // Candidate 1 is better if it has strictly more attributes and
-    // the common sequence is identical.
-    if (Cand1Attrs.size() <= Cand2Attrs.size())
-      return false;
-
-    auto Cand1I = Cand1Attrs.begin();
-    for (auto &Cand2A : Cand2Attrs) {
-      auto &Cand1A = *Cand1I++;
-      llvm::FoldingSetNodeID Cand1ID, Cand2ID;
-      cast<EnableIfAttr>(Cand1A)->getCond()->Profile(Cand1ID,
-                                                     S.getASTContext(), true);
-      cast<EnableIfAttr>(Cand2A)->getCond()->Profile(Cand2ID,
-                                                     S.getASTContext(), true);
-      if (Cand1ID != Cand2ID)
-        return false;
-    }
-
-    return true;
-  }
+       Cand2.Function->hasAttr<EnableIfAttr>()))
+    return hasBetterEnableIfAttrs(S, Cand1.Function, Cand2.Function);
 
   if (S.getLangOpts().CUDA && S.getLangOpts().CUDATargetOverloads &&
       Cand1.Function && Cand2.Function) {
     FunctionDecl *Caller = dyn_cast<FunctionDecl>(S.CurContext);
     return S.IdentifyCUDAPreference(Caller, Cand1.Function) >
            S.IdentifyCUDAPreference(Caller, Cand2.Function);
   }
 
AddressOfFunctionResolver(Sema &S, Expr *SourceExpr,
     }
     
     if (OvlExpr->hasExplicitTemplateArgs())
       OvlExpr->getExplicitTemplateArgs().copyInto(OvlExplicitTemplateArgs);
 
     if (FindAllFunctionsThatMatchTargetTypeExactly()) {
       // C++ [over.over]p4:
       //   If more than one function is selected, [...]
-      if (Matches.size() > 1) {
+      if (Matches.size() > 1 && !eliminiateSuboptimalOverloadCandidates()) {
         if (FoundNonTemplateFunction)
           EliminateAllTemplateMatches();
         else
           EliminateAllExceptMostSpecializedTemplate();
       }
     }
 
     if (S.getLangOpts().CUDA && S.getLangOpts().CUDATargetOverloads &&
         Matches.size() > 1)
       EliminateSuboptimalCudaMatches();
   }
 
   bool hasComplained() const { return HasComplained; }
 
 private:
+  // Is A considered a better overload candidate for the desired type than B?
+  bool isBetterCandidate(const FunctionDecl *A, const FunctionDecl *B) {
+    return hasBetterEnableIfAttrs(S, A, B);
+  }
+
+  // Returns true if we've eliminated any (read: all but one) candidates, false
+  // otherwise.
+  bool eliminiateSuboptimalOverloadCandidates() {
+    // Same algorithm as overload resolution -- one pass to pick the "best",
+    // another pass to be sure that nothing is better than the best.
+    auto Best = Matches.begin();
+    for (auto I = Matches.begin()+1, E = Matches.end(); I != E; ++I)
+      if (isBetterCandidate(I->second, Best->second))
+        Best = I;
+
+    const FunctionDecl *BestFn = Best->second;
+    auto IsBestOrInferiorToBest = [this, BestFn](
+        const std::pair<DeclAccessPair, FunctionDecl *> &Pair) {
+      return BestFn == Pair.second || isBetterCandidate(BestFn, Pair.second);
+    };
+
+    // Note: We explicitly leave Matches unmodified if there isn't a clear best
+    // option, so we can potentially give the user a better error
+    if (!std::all_of(Matches.begin(), Matches.end(), IsBestOrInferiorToBest))
+      return false;
+    Matches[0] = *Best;
+    Matches.resize(1);
+    return true;
+  }
+
   bool isTargetTypeAFunction() const {
     return TargetFunctionType->isFunctionType();
   }
 
   // [ToType]     [Return]
 
   // R (*)(A) --> R (A), IsNonStaticMemberFunction = false
   // R (&)(A) --> R (A), IsNonStaticMemberFunction = false

cfe/trunk/test/CodeGen/enable_if.c

void test2() {
   void *vp1 = (void*)&bar;
   // CHECK: store i8* bitcast (void (i32)* @_Z3barUa9enable_ifIXLi1EEEi to i8*)
   void *vp2 = (void*)bar;
   // CHECK: store i8* bitcast (void (i32)* @_Z3barUa9enable_ifIXLi1EEEi to i8*)
   vp1 = (void*)&bar;
   // CHECK: store i8* bitcast (void (i32)* @_Z3barUa9enable_ifIXLi1EEEi to i8*)
   vp1 = (void*)bar;
 }
+
+void baz(int m) __attribute__((overloadable, enable_if(1, "")));
+void baz(int m) __attribute__((overloadable));
+// CHECK-LABEL: define void @test3
+void test3() {
+  // CHECK: store void (i32)* @_Z3bazUa9enable_ifIXLi1EEEi
+  void (*p)(int) = baz;
+  // CHECK: store void (i32)* @_Z3bazUa9enable_ifIXLi1EEEi
+  void (*p2)(int) = &baz;
+  // CHECK: store void (i32)* @_Z3bazUa9enable_ifIXLi1EEEi
+  p = baz;
+  // CHECK: store void (i32)* @_Z3bazUa9enable_ifIXLi1EEEi
+  p = &baz;
+}
+
+
+const int TRUEFACTS = 1;
+void qux(int m) __attribute__((overloadable, enable_if(1, ""),
+                               enable_if(TRUEFACTS, "")));
+void qux(int m) __attribute__((overloadable, enable_if(1, "")));
+// CHECK-LABEL: define void @test4
+void test4() {
+  // CHECK: store void (i32)* @_Z3quxUa9enable_ifIXLi1EEXL_Z9TRUEFACTSEEEi
+  void (*p)(int) = qux;
+  // CHECK: store void (i32)* @_Z3quxUa9enable_ifIXLi1EEXL_Z9TRUEFACTSEEEi
+  void (*p2)(int) = &qux;
+  // CHECK: store void (i32)* @_Z3quxUa9enable_ifIXLi1EEXL_Z9TRUEFACTSEEEi
+  p = qux;
+  // CHECK: store void (i32)* @_Z3quxUa9enable_ifIXLi1EEXL_Z9TRUEFACTSEEEi
+  p = &qux;
+}

cfe/trunk/test/CodeGenCXX/enable_if.cpp

 // RUN: %clang_cc1 -emit-llvm %s -o - -triple=x86_64-pc-linux-gnu| FileCheck %s
+
+// Test address-of overloading logic
+int test5(int);
+template <typename T>
+T test5(T) __attribute__((enable_if(1, "better than non-template")));
+
+// CHECK: @_Z5test5IiEUa9enable_ifIXLi1EEET_S0_
+int (*Ptr)(int) = &test5;
+
 // Test itanium mangling for attribute enable_if
 
 // CHECK: _Z5test1Ua9enable_ifIXeqfL0p_Li1EEEi
 void test1(int i) __attribute__((enable_if(i == 1, ""))) {}
 
 void ext();
 // CHECK: _Z5test2Ua9enable_ifIXneadL_Z3extvELi0EEEi
 void test2(int i) __attribute__((enable_if(&ext != 0, ""))) {}

cfe/trunk/test/Sema/enable_if.c

 // RUN: %clang_cc1 %s -verify
 // RUN: %clang_cc1 %s -DCODEGEN -emit-llvm -o - | FileCheck %s
 
 #define O_CREAT 0x100
 typedef int mode_t;
 typedef unsigned long size_t;
 
+const int TRUE = 1;
+
 int open(const char *pathname, int flags) __attribute__((enable_if(!(flags & O_CREAT), "must specify mode when using O_CREAT"))) __attribute__((overloadable));  // expected-note{{candidate disabled: must specify mode when using O_CREAT}}
 int open(const char *pathname, int flags, mode_t mode) __attribute__((overloadable));  // expected-note{{candidate function not viable: requires 3 arguments, but 2 were provided}}
 
 void test1() {
 #ifndef CODEGEN
   open("path", O_CREAT);  // expected-error{{no matching function for call to 'open'}}
 #endif
   open("path", O_CREAT, 0660);

 void f(int n) __attribute__((enable_if(global == 0, "chosen when 'global' is zero")));  // expected-error{{'enable_if' attribute expression never produces a constant expression}}  // expected-note{{subexpression not valid in a constant expression}}
 
 const int cst = 7;
 void return_cst(void) __attribute__((overloadable)) __attribute__((enable_if(cst == 7, "chosen when 'cst' is 7")));
 void test_return_cst() { return_cst(); }
 
 void f2(void) __attribute__((overloadable)) __attribute__((enable_if(1, "always chosen")));
 void f2(void) __attribute__((overloadable)) __attribute__((enable_if(0, "never chosen")));
-void f2(void) __attribute__((overloadable));
+void f2(void) __attribute__((overloadable)) __attribute__((enable_if(TRUE, "always chosen #2")));
 void test6() {
-  void (*p1)(void) = &f2; // expected-error{{initializing 'void (*)(void)' with an expression of incompatible type '<overloaded function type>'}} expected-note@119{{candidate function}} expected-note@120{{candidate function made ineligible by enable_if}} expected-note@121{{candidate function}}
-  void (*p2)(void) = f2; // expected-error{{initializing 'void (*)(void)' with an expression of incompatible type '<overloaded function type>'}} expected-note@119{{candidate function}} expected-note@120{{candidate function made ineligible by enable_if}} expected-note@121{{candidate function}}
-  void *p3 = (void*)&f2; // expected-error{{address of overloaded function 'f2' is ambiguous}} expected-note@119{{candidate function}} expected-note@120{{candidate function made ineligible by enable_if}} expected-note@121{{candidate function}}
-  void *p4 = (void*)f2; // expected-error{{address of overloaded function 'f2' is ambiguous}} expected-note@119{{candidate function}} expected-note@120{{candidate function made ineligible by enable_if}} expected-note@121{{candidate function}}
+  void (*p1)(void) = &f2; // expected-error{{initializing 'void (*)(void)' with an expression of incompatible type '<overloaded function type>'}} expected-note@121{{candidate function}} expected-note@122{{candidate function made ineligible by enable_if}} expected-note@123{{candidate function}}
+  void (*p2)(void) = f2; // expected-error{{initializing 'void (*)(void)' with an expression of incompatible type '<overloaded function type>'}} expected-note@121{{candidate function}} expected-note@122{{candidate function made ineligible by enable_if}} expected-note@123{{candidate function}}
+  void *p3 = (void*)&f2; // expected-error{{address of overloaded function 'f2' is ambiguous}} expected-note@121{{candidate function}} expected-note@122{{candidate function made ineligible by enable_if}} expected-note@123{{candidate function}}
+  void *p4 = (void*)f2; // expected-error{{address of overloaded function 'f2' is ambiguous}} expected-note@121{{candidate function}} expected-note@122{{candidate function made ineligible by enable_if}} expected-note@123{{candidate function}}
 }
 
 void f3(int m) __attribute__((overloadable)) __attribute__((enable_if(m >= 0, "positive")));
 void f3(int m) __attribute__((overloadable)) __attribute__((enable_if(m < 0, "negative")));
 void test7() {
-  void (*p1)(int) = &f3; // expected-error{{initializing 'void (*)(int)' with an expression of incompatible type '<overloaded function type>'}} expected-note@129{{candidate function made ineligible by enable_if}} expected-note@130{{candidate function made ineligible by enable_if}}
-  void (*p2)(int) = f3; // expected-error{{initializing 'void (*)(int)' with an expression of incompatible type '<overloaded function type>'}} expected-note@129{{candidate function made ineligible by enable_if}} expected-note@130{{candidate function made ineligible by enable_if}}
-  void *p3 = (void*)&f3; // expected-error{{address of overloaded function 'f3' does not match required type 'void'}} expected-note@129{{candidate function made ineligible by enable_if}} expected-note@130{{candidate function made ineligible by enable_if}}
-  void *p4 = (void*)f3; // expected-error{{address of overloaded function 'f3' does not match required type 'void'}} expected-note@129{{candidate function made ineligible by enable_if}} expected-note@130{{candidate function made ineligible by enable_if}}
+  void (*p1)(int) = &f3; // expected-error{{initializing 'void (*)(int)' with an expression of incompatible type '<overloaded function type>'}} expected-note@131{{candidate function made ineligible by enable_if}} expected-note@132{{candidate function made ineligible by enable_if}}
+  void (*p2)(int) = f3; // expected-error{{initializing 'void (*)(int)' with an expression of incompatible type '<overloaded function type>'}} expected-note@131{{candidate function made ineligible by enable_if}} expected-note@132{{candidate function made ineligible by enable_if}}
+  void *p3 = (void*)&f3; // expected-error{{address of overloaded function 'f3' does not match required type 'void'}} expected-note@131{{candidate function made ineligible by enable_if}} expected-note@132{{candidate function made ineligible by enable_if}}
+  void *p4 = (void*)f3; // expected-error{{address of overloaded function 'f3' does not match required type 'void'}} expected-note@131{{candidate function made ineligible by enable_if}} expected-note@132{{candidate function made ineligible by enable_if}}
 }
 #endif

cfe/trunk/test/SemaCXX/enable_if.cpp

void test2() {
     int (*p)(int) = ovlBar;
     int (*p2)(int) = &ovlBar;
     int (*a)(int);
     a = ovlBar;
     a = &ovlBar;
   }
 
   int ovlConflict(int m) __attribute__((enable_if(true, "")));
-  int ovlConflict(int m);
+  int ovlConflict(int m) __attribute__((enable_if(1, "")));
   void test3() {
     int (*p)(int) = ovlConflict; // expected-error{{address of overloaded function 'ovlConflict' is ambiguous}} expected-note@191{{candidate function}} expected-note@192{{candidate function}}
     int (*p2)(int) = &ovlConflict; // expected-error{{address of overloaded function 'ovlConflict' is ambiguous}} expected-note@191{{candidate function}} expected-note@192{{candidate function}}
     int (*a)(int);
     a = ovlConflict; // expected-error{{assigning to 'int (*)(int)' from incompatible type '<overloaded function type>'}} expected-note@191{{candidate function}} expected-note@192{{candidate function}}
     a = &ovlConflict; // expected-error{{assigning to 'int (*)(int)' from incompatible type '<overloaded function type>'}} expected-note@191{{candidate function}} expected-note@192{{candidate function}}
   }
 
void test5() {
     a = &templatedBar<int>; // expected-error{{assigning to 'int (*)(int)' from incompatible type '<overloaded function type>'}} expected-note@214{{candidate function made ineligible by enable_if}}
   }
 
   template <typename T>
   T templatedConflict(T m) __attribute__((enable_if(false, ""))) { return T(); }
   template <typename T>
   T templatedConflict(T m) __attribute__((enable_if(true, ""))) { return T(); }
   template <typename T>
-  T templatedConflict(T m) { return T(); }
+  T templatedConflict(T m) __attribute__((enable_if(1, ""))) { return T(); }
   void test6() {
     int (*p)(int) = templatedConflict<int>; // expected-error{{address of overloaded function 'templatedConflict' is ambiguous}} expected-note@224{{candidate function made ineligible by enable_if}} expected-note@226{{candidate function}} expected-note@228{{candidate function}}
     int (*p0)(int) = &templatedConflict<int>; // expected-error{{address of overloaded function 'templatedConflict' is ambiguous}} expected-note@224{{candidate function made ineligible by enable_if}} expected-note@226{{candidate function}} expected-note@228{{candidate function}}
     int (*a)(int);
     a = templatedConflict<int>; // expected-error{{assigning to 'int (*)(int)' from incompatible type '<overloaded function type>'}} expected-note@226{{candidate function}} expected-note@228{{candidate function}}
     a = &templatedConflict<int>; // expected-error{{assigning to 'int (*)(int)' from incompatible type '<overloaded function type>'}} expected-note@226{{candidate function}} expected-note@228{{candidate function}}
   }
 }