This is an archive of the discontinued LLVM Phabricator instance.

[Sema] Change order of displayed overloads in diagnostics
ClosedPublic

Authored by ilya-biryukov on Sep 1 2023, 4:50 AM.

Details

Summary

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 is 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.

Diff Detail

Event Timeline

ilya-biryukov created this revision.Sep 1 2023, 4:50 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 1 2023, 4:50 AM
ilya-biryukov requested review of this revision.Sep 1 2023, 4:50 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 1 2023, 4:50 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
ilya-biryukov added reviewers: rnk, Restricted Project.Sep 1 2023, 4:53 AM
ilya-biryukov added a subscriber: rnk.

This change mainly tried to address the assertion failure in libc++, but got into the territory of changing the order of displayed overloads.
Which might be a more complicated topic, so it would be great to get feedback from Clang maintainers.

I am on vacation next week, so @rnk might want pick this up if fixing the assertion turns out to be urgent.

rnk added a subscriber: cjdb.Sep 18 2023, 9:38 AM

+@cjdb , do you have time to review this?

@cor3ntin any suggestions on how to proceed here?

I hope the approach taken in the current patch should already provide meaningful and improved results in most cases.
It only affects the diagnostics output, so changing or reverting it should be relatively easy in the future.

I'm happy to ask someone from my team to do the detailed code review, but I just wanted to make sure the Clang contributors are generally ok with the direction here.

shafik added a subscriber: shafik.Oct 10 2023, 6:25 PM

You say "attempts to be a strict weak order" does that mean there are still cases which will cause an assert or are we not sure?

You say "attempts to be a strict weak order" does that mean there are still cases which will cause an assert or are we not sure?

No, it's an actual strict weak order. It's a bad choice of wording on my part.

ilya-biryukov edited the summary of this revision. (Show Details)Oct 11 2023, 2:37 AM
aaron.ballman accepted this revision.Oct 11 2023, 6:24 AM

I think the changes should come with a release note so users know about the improved user experience (it would be great to show the code example from this patch summary in the release notes with the explanation you gave). Overall, I think this gives reasonable diagnostic behavior (I expect it to roughly be a wash in terms of showing the "best" order -- some will improve, some will degrade) but the ability to make the sets sortable without triggering checked STL diagnostics is definitely an improvement. Thanks!

clang/lib/Sema/SemaOverload.cpp
11817–11819

Coding style nit.

11872–11874
11927–11928
11955
This revision is now accepted and ready to land.Oct 11 2023, 6:24 AM
ilya-biryukov marked 4 inline comments as done.
  • Addressed comments
  • Added a release note
This revision was landed with ongoing or failed builds.Oct 23 2023, 4:06 AM
This revision was automatically updated to reflect the committed changes.