[PATCH] D158472: [clang][Diagnostics] Emit fix-it hint separately on overload resolution failure

[Takuya Shimizu via Phabricator via cfe-commits]

hazohelet abandoned this revision.
hazohelet added a comment.

In D158472#4645378 <https://reviews.llvm.org/D158472#4645378>, @aaron.ballman wrote:

> In D158472#4605228 <https://reviews.llvm.org/D158472#4605228>, @hazohelet wrote:
>
>> This breaks the one-note-for-one-overload-candidate rule of overload resolution failure diagnostics (https://github.com/llvm/llvm-project/blob/ff08c8e57e39d7970b65637595cdc221901f4ed1/clang/lib/Sema/SemaOverload.cpp#L11517-L11526), but in most cases this change would produce less lines of diagnostics.
>> If we don't like this additional note, we could instead suppress the fix-it diagnostics when the fix-it location and the diagnosed candidate location are not close (for example, more than 2 lines away).
>
> I'm uncomfortable with starting down the slippery slope of multiple notes per overload resolution failure; that leads to an explosion of extra diagnostics which makes it harder to spot the issue in the first place. My inclination is to try to resolve this by fixing the way we emit fix-it diagnostics so that we remove the extra whitespace from there because this seems like it's a general problem with fix-its. Or am I wrong about this being a general issue? CC @cjdb for opinions as well

I agree that the ideal fix would be a systematic change in how we display fix-it hints, or for that matter, distant source ranges in general.
We probably want output that looks like this:

  <source>:1:6: note: candidate function not viable: no known conversion from 'int' to 'int *' for 1st argument; take the address of the argument with &
      1 | void ptr(int *p);
        |      ^   ~~~~~~
      ---
      5 |   ptr(n);
        |       
        |       &

In any case, suppressing fix-it hints would be disastrous for LSP things like clangd that use clang diagnostic output. So, this is a problem of how clang diagnostic engine display fix-it hints.



================
Comment at: clang/test/Sema/overloadable.c:31
+  accept_funcptr(f2); // expected-error{{no matching function for call to 'accept_funcptr'}} \
+                      // expected-note 2{{take the address of the argument with &}}
 }
----------------
aaron.ballman wrote:
> Something odd is going on -- why is the note emitted twice?
There are two candidates, both of which would become viable if we take the address of them. (https://godbolt.org/z/6onn9x89a)


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D158472/new/

https://reviews.llvm.org/D158472