EricWF marked 6 inline comments as done.
EricWF added inline comments.
================
Comment at: lib/Sema/SemaOverload.cpp:8888
+ bool PerformADL) {
+ assert(getLangOpts().CPlusPlus2a);
+ auto Opc = BinaryOperator::getOverloadedOpcode(Op);
----------------
rsmith wrote:
> Does the implementation below actually rely on this?
Like should it be possible to hit this line of code? No. It was preemptive.
I'll remove it.
================
Comment at: lib/Sema/SemaOverload.cpp:12537-12565
+/// Rewritten candidates have been added but not checked for validity. They
+/// could still be non-viable if:
+/// (A) The rewritten call (x <=> y) is a builtin, but it will be ill-formed
+/// when built (for example it has narrowing conversions).
+/// (B) The expression (x <=> y) @ 0 is ill-formed for the result of (x <=>
y).
+///
+/// If either is the case, this function should be considered non-viable and
----------------
rsmith wrote:
> Hmm, this is an interesting approach. It's quite a divergence from our usual
> approach, which is to perform the viability checks first, before partial
> ordering, even if we could eliminate some candidates based on partial
> ordering. And it's observable -- it will result in fewer template
> instantiations being performed, which will mean we do not diagnose some
> ill-formed (no diagnostic required) situations which would otherwise fail due
> to an error in a non-immediate context.
>
> Can you say something about why you chose this approach instead of doing the
> lookup for operator `@` early and marking the candidate non-viable if the
> lookup / overload resolution for it fails?
>
> (I'm also vaguely wondering whether we should be pushing back on the C++
> committee for making viability of the rewritten candidate depend on validity
> of the `@` expression, not just the `<=>` expression.)
The main reason for choosing this approach was the current expense of
validating the candidates eagerly when they're added. Currently builtin
candidates need to be fully built before their viability is known. My initial
attempt at this made the compiler just about hang.
If I understand what you mean by performing lookup/overload resolution early
for operator `@`, and I probably don't, then it would require performing
overload resolution separately every rewritten candidate added, since each
candidate could have a different return type.
I think rewritten builtins could be checked more eagerly by moving a bunch of
the checking from `SemaExpr.cpp` to `SemaOverload.cpp`, and using it to eagerly
check the bits we need to, but not actually building the expression as calling
into `SemaExpr.cpp` would currently do.
As for the root question: Should the validity of the `@` expression be
considered during overload resolution? I think the answer is tricky. For
example, the following case could easily occur and break existing code,
especially as more users add defaulted comparison operators. In this case if
the base class adds <=> without the derived classes knowledge.
```
struct B {
using CallbackTy = void(*)();
CallbackTy CB = nullptr; // B carries around a function pointer.
#if __cplusplus >= 201703L
friend auto operator<=>(B const&, B const&) = default;
#else
friend bool operator==(B, B);
friend bool operator!=(B, B);
#endif
};
struct D {
operator int() const; // D uses this conversion to provide comparisons.
};
D d;
// comparison ill-formed if the `@` expression isn't considered for viability.
auto r = d < d;
```
However, allowing the `@` expression to effect viability leads to a different
set of bugs. For example this slicing comparison bug:
```
struct B {
int x;
friend auto operator<=>(B, B) = default;
};
struct D : B {
decltype(nullptr) y = {};
friend auto operator<=>(D, D) = default;
};
D d;
// If the `@` expression is considered for viability, then `operator<=>(B, B)`
will be used, effectively slicing the data used during the comparison.
auto r = d < d;
```
What is clear is that it should go back to CWG for more discussion.
================
Comment at: lib/Sema/SemaOverload.cpp:12570-12572
+ case OR_Deleted:
+ // FIXME(EricWF): If we've found a deleted rewritten operator, it's
+ // possible we should have never considered it a viable candidate.
----------------
rsmith wrote:
> This is a really interesting situation. What does it even mean to consider
> the `(a <=> b) @ c` expression if lookup for `<=>` finds a deleted function?
> (It would be novel for the return type of a deleted function to be
> meaningful, and it would be novel to exclude an overload candidate because
> it's deleted.) And what if lookup for `@` finds a deleted function?
>
> I think this is good reason to go back to CWG and suggest that we don't check
> the `@` expression at all until after overload resolution.
> What does it even mean to consider the (a <=> b) @ c expression if lookup for
> <=> finds a deleted function?
Separate from allowing return types to affect overload resolution, I suspect
selecting a deleted rewritten candidate means that we should have never
considered the candidate in the first place. For example:
```
struct B { auto operator<=>(B) const = delete; };
struct D : private B { operator int() const; };
D{} < D{};
```
In the above case, the intent of the code seems clear. But because the compiler
invented a `<=>` candidate with a better conversion ranking, our code is
ill-formed. Therefore, it makes sense to prefer non-rewritten candidates over
deleted rewritten ones.
That said, it certainly doesn't make sense to "make way" for a worse rewritten
candidate when the best viable function is deleted. For example:
```
struct Any { Any(...); };
auto operator<=>(Any, Any);
struct T { friend auto operator<=>(T, T) = delete; };
T{} < T{}; // selecting operator<=>(Any, Any) would be bad.
```
================
Comment at: lib/Sema/SemaOverload.cpp:12608-12609
+ };
+ // Sort the candidate functions based on their partial ordering,
+ // and find the first N functions which rank equally.
+ std::sort(Overloads.begin(), Overloads.end(), CmpOverloads);
----------------
rsmith wrote:
> This is not correct, because it turns out that `isBetterOverloadCandidate` is
> not a strict weak order. It's not any kind of order at all, actually, since
> it lacks the transitivity property. I don't know whether you can do better
> than marking your chosen candidate non-viable and rerunning the candidate
> selection algorithm if your presumed-best candidate turns out to be
> non-viable.
I was afraid of that, but I bit too tired to initially check this. Thanks for
the correction.
If I'm not mistaken, `isBetterOverloadCandidate` should still be able to
provide list of equivalent candidates, which could be used when handling this
case.
================
Comment at: lib/Sema/SemaOverload.cpp:12634
+ }
+ // If none of the rewritten
+ if (NumViableCandidates > 1)
----------------
rsmith wrote:
> I am a vampire and
Ha. I had to look that reference up.
Repository:
rC Clang
https://reviews.llvm.org/D45680
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
