Page MenuHomePhabricator

[clang] Improve diagnostic note for implicit conversions that are disallowed because they involve more than one user-defined conversion.
AbandonedPublic

Authored by logan-5 on Feb 4 2020, 4:16 PM.

Details

Reviewers
rsmith
jkorous
Summary

The current text of the 'note' diagnostic for bad conversions is confusing in the presence of user-defined conversion operations: https://godbolt.org/z/zrgeHH
For the first error, the conversion function is simply pointed at in a note without any further explanation. For the second error, the final note claims there is no conversion from X2 to Y2, even though it is plainly visible in the source code. The reason for the error is that only one implicit user-defined conversion may be applied in these circumstances, but the error message is misleading to those who may not know this rule.

This patch clarifies this situation with a better diagnostic message. It does so by finding user conversions even where they are not allowed, and then discarding them afterward as invalid, but keeping a record that they existed. There is also a stronger mode (UDC_Skip) for skipping the search for user-defined conversions outright, which matches the old behavior and is retained to prevent infinite recursions.

Diff Detail

Event Timeline

logan-5 created this revision.Feb 4 2020, 4:16 PM
jkorous added a subscriber: jkorous.

Hi @logan-5!
I suggest you split the patch into two smaller ones so it is easier to review.

  1. A NFC patch with refactoring of the interface (bool -> UserDefinedConversionsKind).
  2. Patch that introduces and uses diag::note_ovl_candidate_bad_user_defined_conv.
rsmith added a comment.Feb 4 2020, 5:39 PM

This is an interesting idea. Have you looked into whether you can defer the check for conversion via multiple user-defined conversions until we come to call CompleteNonViableCandidate after finding no viable functions? That'd give me a lot more confidence that this won't introduce a noticeable performance regression for pathological cases (eg, considering a quadratic -- or worse -- number of combinations of constructors and conversion functions).

clang/include/clang/Basic/DiagnosticSemaKinds.td
4068

applies -> would apply

clang/include/clang/Sema/Sema.h
2947–2956

Please use an enum class here, so that call sites are more meaningful (UserDefinedConversions::Allow vs UDC_Allow).

Have you looked into whether you can defer the check for conversion via multiple user-defined conversions until we come to call CompleteNonViableCandidate after finding no viable functions?

That's a good idea, and does seem more efficient than checking for multiple conversions every time. I'll look into that, and also split this patch into two as @jkorous suggested.

logan-5 abandoned this revision.Feb 7 2020, 10:16 AM

I split this patch into multiple, and reworked the implementation. The new stuff lives here and here and here: D74238 D74234 D74235