ilya-biryukov created this revision.
Herald added a project: All.
ilya-biryukov requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.
Make it a strict weak order.
Fixes #64121.
Current implementation uses the definition the definition of ordering
in the C++ standard. The definition provides only a partial order and
cannot be used in sorting algorithms.
The debug builds of libc++ are capable of detecting that problem
and this failure was found when building Clang with libc++ and
those extra checks enabled, see #64121.
The new ordering takes attempts to be a strict weak order and still
pushes most interesting functions to the start of the list.
In some cases, it leads to better results, e.g.
struct Foo {
operator int();
operator const char*();
};
void test() { Foo() - Foo(); }
Now produces a list with two most relevant builtin operators at the top,
i.e. `operator-(int, int)` and `operator-(const char*, const char*)`.
Previously `operator-(const char*, const char*)` was the first element,
but `operator-(int, int)` was only the 13th element in the output.
This is a consequence of `stable_sort` now being able to compare those
two candidates, which are indistinguishable in the semantic partial order
despite being two local minimums in their respective comparable
subsets.
However, new implementation does not take into account some aspects of
C++ semantics, e.g. which function template is more specialized. This
can also lead to worse ordering sometimes.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D159351
Files:
clang/lib/Sema/SemaOverload.cpp
clang/test/SemaCXX/overloaded-operators-display-order-crash.cpp
Index: clang/test/SemaCXX/overloaded-operators-display-order-crash.cpp
===================================================================
--- /dev/null
+++ clang/test/SemaCXX/overloaded-operators-display-order-crash.cpp
@@ -0,0 +1,35 @@
+// RUN: %clang_cc1 -fsyntax-only -verify %s
+
+namespace ambig {
+ struct Foo {
+ operator int();
+ operator const char *();
+ };
+
+
+ void func(const char*, long);
+ void func(const char*, const char*);
+ void func(int, int);
+
+ bool doit(Foo x) {
+ func(x, x); // expected-error {{call to 'func' is ambiguous}}
+ // expected-note@* 3{{candidate}}
+ }
+}
+
+namespace bad_conversion {
+ struct Foo {
+ operator int();
+ operator const char *();
+ };
+
+
+ void func(double*, const char*, long);
+ void func(double*, const char*, const char*);
+ void func(double*, int, int);
+
+ bool doit(Foo x) {
+ func((int*)0, x, x); // expected-error {{no matching function for call to 'func'}}
+ // expected-note@* 3{{candidate}}
+ }
+}
Index: clang/lib/Sema/SemaOverload.cpp
===================================================================
--- clang/lib/Sema/SemaOverload.cpp
+++ clang/lib/Sema/SemaOverload.cpp
@@ -38,8 +38,10 @@
#include "llvm/ADT/STLExtras.h"
#include "llvm/ADT/SmallPtrSet.h"
#include "llvm/ADT/SmallString.h"
+#include "llvm/ADT/SmallVector.h"
#include "llvm/Support/Casting.h"
#include <algorithm>
+#include <cstddef>
#include <cstdlib>
#include <optional>
@@ -11774,6 +11776,7 @@
}
namespace {
+
struct CompareOverloadCandidatesForDisplay {
Sema &S;
SourceLocation Loc;
@@ -11811,13 +11814,10 @@
if (L->Viable) {
if (!R->Viable) return true;
- // TODO: introduce a tri-valued comparison for overload
- // candidates. Would be more worthwhile if we had a sort
- // that could exploit it.
- if (isBetterOverloadCandidate(S, *L, *R, SourceLocation(), CSK))
- return true;
- if (isBetterOverloadCandidate(S, *R, *L, SourceLocation(), CSK))
- return false;
+ if (int Ord = CompareConversions(*L, *R)) {
+ return Ord < 0;
+ }
+ // Use other tie breakers.
} else if (R->Viable)
return false;
@@ -11869,30 +11869,9 @@
}
// If there's any ordering between the defined conversions...
- // FIXME: this might not be transitive.
- assert(L->Conversions.size() == R->Conversions.size());
-
- int leftBetter = 0;
- unsigned I = (L->IgnoreObjectArgument || R->IgnoreObjectArgument);
- for (unsigned E = L->Conversions.size(); I != E; ++I) {
- switch (CompareImplicitConversionSequences(S, Loc,
- L->Conversions[I],
- R->Conversions[I])) {
- case ImplicitConversionSequence::Better:
- leftBetter++;
- break;
-
- case ImplicitConversionSequence::Worse:
- leftBetter--;
- break;
-
- case ImplicitConversionSequence::Indistinguishable:
- break;
- }
+ if (int Ord = CompareConversions(*L, *R)) {
+ return Ord < 0;
}
- if (leftBetter > 0) return true;
- if (leftBetter < 0) return false;
-
} else if (RFailureKind == ovl_fail_bad_conversion)
return false;
@@ -11914,10 +11893,67 @@
SourceLocation RLoc = GetLocationForCandidate(R);
// Put candidates without locations (e.g. builtins) at the end.
- if (LLoc.isInvalid()) return false;
- if (RLoc.isInvalid()) return true;
+ if (LLoc.isValid() && RLoc.isValid())
+ return S.SourceMgr.isBeforeInTranslationUnit(LLoc, RLoc);
+ if (LLoc.isValid() && !RLoc.isValid())
+ return true;
+ if (RLoc.isValid() && !LLoc.isValid())
+ return false;
+ assert(!LLoc.isValid() && !RLoc.isValid());
+ // For builtins and other functions without locations, fallback to the order
+ // in which they were added into the candidate set.
+ return L < R;
+ }
- return S.SourceMgr.isBeforeInTranslationUnit(LLoc, RLoc);
+private:
+ struct ConversionSignals {
+ unsigned KindRank = 0;
+ ImplicitConversionRank Rank = ICR_Exact_Match;
+
+ static ConversionSignals ForSequence(ImplicitConversionSequence &Seq) {
+ ConversionSignals Sig;
+ Sig.KindRank = Seq.getKindRank();
+ if (Seq.isStandard())
+ Sig.Rank = Seq.Standard.getRank();
+ else if (Seq.isUserDefined())
+ Sig.Rank = Seq.UserDefined.After.getRank();
+ // We intend StaticObjectArgumentConversion to compare the same as
+ // StandardConversion with ICR_ExactMatch rank.
+ return Sig;
+ }
+
+ static ConversionSignals ForObjectArgument() {
+ // We intend StaticObjectArgumentConversion to compare the same as
+ // StandardConversion with ICR_ExactMatch rank.
+ // Default give us that.
+ return {};
+ }
+ };
+
+ // Returns -1 if conversions in L are considered better.
+ // 0 if they are considered indistinguishable.
+ // 1 if conversions in R are better.
+ int CompareConversions(const OverloadCandidate &L,
+ const OverloadCandidate &R) {
+ // We cannot use `isBetterOverloadCandidate` because it is defined
+ // according to the C++ standard and provides a partial order, but we need
+ // a total order as this function is used in sort.
+ assert(L.Conversions.size() == R.Conversions.size());
+ for (unsigned I = 0, N = L.Conversions.size(); I != N; ++I) {
+ auto LS = L.IgnoreObjectArgument && I == 0
+ ? ConversionSignals::ForObjectArgument()
+ : ConversionSignals::ForSequence(L.Conversions[I]);
+ auto RS = R.IgnoreObjectArgument
+ ? ConversionSignals::ForObjectArgument()
+ : ConversionSignals::ForSequence(R.Conversions[I]);
+ if (std::tie(LS.KindRank, LS.Rank) != std::tie(RS.KindRank, RS.Rank))
+ return std::tie(LS.KindRank, LS.Rank) < std::tie(RS.KindRank, RS.Rank)
+ ? -1
+ : 1;
+ }
+ // FIXME: find a way to compare templates for being more or less
+ // specialized that provides a weak ordering.
+ return 0;
}
};
}
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
