[cfe-dev] Alternative FixItHints

Ben Jackson via cfe-dev cfe-dev at lists.llvm.org
Sun Jun 19 16:24:00 PDT 2016


Just following up on this thread: Yes it totally makes sense.

Previously we were ignoring any fixit hints attached to “child” diagnostics (in libclang parlance). I’ve made a quick change to YCM which:
 - extracts fixit hints from chid diagnostics (typically notes)
 - provides a UI in Vim to select between them when multiple fixits are available for a given diag.

It works nicely for the case you mention below: https://files.gitter.im/Valloric/YouCompleteMe/l6FD/YCM-fixit-multiple-choice2.gif <https://files.gitter.im/Valloric/YouCompleteMe/l6FD/YCM-fixit-multiple-choice2.gif>. Incidentally, this also improves the workflow when there are multiple diagnostics on the line.

With regard to Manuel’s question about who should be responsible for choosing the insertion location for, say includes, I’d be keen for clang to make that decision, as it has better knowledge about code than the IDE (consider objective-c vs. c vs…). It also requires less work from each client implementation.

That said, in YCM we already have some stuff for working out where to put automatically generated import declarations for c-sharp, and we even provide hooks for the user to tune to their taste, so there is an argument that where the decision is arbitrary, the client should offer the user more flexibility.

On 2 Jun 2016, at 12:00, Benjamin Kramer <benny.kra at gmail.com> wrote:
> 
> Ben, would it make sense for YCM to provide the user with choice when
> there are notes attached to a diagnostic? You can see this in action
> for -Wparentheses:
> 
> $ cat t.c
> void f(int foo, int bar) {
>  if (foo = bar) {}
> }
> 
> $ clang t.c
> t.c:2:11: warning: using the result of an assignment as a condition without
>      parentheses [-Wparentheses]
>  if (foo = bar) {}
>      ~~~~^~~~~
> t.c:2:11: note: place parentheses around the assignment to silence this
>      warning
>  if (foo = bar) {}
>          ^
>      (        )
> t.c:2:11: note: use '==' to turn this assignment into an equality comparison
>  if (foo = bar) {}
>          ^
>          ==
> 
> On Wed, Jun 1, 2016 at 8:59 PM, Richard Smith <richard at metafoo.co.uk> wrote:
>> On Wed, Jun 1, 2016 at 11:46 AM, Manuel Klimek via cfe-dev
>> <cfe-dev at lists.llvm.org> wrote:
>>> 
>>> On Wed, Jun 1, 2016 at 6:59 PM David Blaikie via cfe-dev
>>> <cfe-dev at lists.llvm.org> wrote:
>>>> 
>>>> I think the general idea in Clang has been that fixits on a warning must
>>>> be behavior-preserving
>>> 
>>> 
>>> Typo-correction is not behavior preserving, though?
>> 
>> 
>> We do not issue typo-correction FixItHints on warnings. (Well, actually, I
>> think we have a single warning that does this, but it's a bug.)
>> 
>>>> (because we must recover from errors (and warnings can be errors) as if
>>>> the fixit were applied, so that downstream diagnostics/fixits are
>>>> applicable). We use notes to express other fixit alternatives (or any
>>>> non-behavior preserving fixits if there's no behavior-preserving fixit we
>>>> care to give). Should we not be using notes as alternatives in the include
>>>> fixer tool?
>> 
>> 
>> +1
>> 
>> This is our usual approach for the case where there are multiple possible
>> fixes: one note per possible approach, with the FixItHints for that approach
>> attached to that note. Is there some reason why this doesn't apply in your
>> case?
>> 
>>>> 
>>>> Is there some semantic information missing from multiple notes with
>>>> fixits that we could improve instead? (that would maintain/improve the
>>>> existing command line usability) to accurately describe which notes form the
>>>> set of alternative solutions? (so that an IDE, etc, could collapse/render
>>>> them differently, perhaps?)
>>>> 
>>>> On Wed, May 25, 2016 at 8:24 AM, Benjamin Kramer via cfe-dev
>>>> <cfe-dev at lists.llvm.org> wrote:
>>>>> 
>>>>> Hi all,
>>>>> 
>>>>> one of the longer-term goals for the include fixer tool is integration
>>>>> into the editor. This could piggy-back on the existing support for
>>>>> FixIts but there is one big missing feature. It is often not perfectly
>>>>> clear what #include a user wants or if it's just a typo that needs
>>>>> correcting, we cannot express that with the current FixIts. A Clang
>>>>> diagnostic can have multiple FixIts but they are meant to be applied
>>>>> together, for this case we'd need alternative groups of FixIts.
>>>>> 
>>>>> There's also a fair number of existing clang warnings that could
>>>>> benefit from this, typo correction is an obvious one but there's also
>>>>> a bunch of disambiguation warnings that can have fix-its in both
>>>>> directions, for example '= in if statement' (turn into '==' or add
>>>>> double parens) or '&& within ||' where parens can be added both ways.
>>>>> The functionality-preserving FixIt is the canonical one for those
>>>>> warnings but having an alternative would be useful, too.
>>>>> 
>>>>> Of course this won't work on the terminal. FixIts are already hardly
>>>>> usable there, so this will be a feature for IDEs, code review tools
>>>>> etc.
>>>>> 
>>>>> My current plan is:
>>>>>  1. Have DiagnosticEngine and friends keep a vector of vector of
>>>>> FixItHint instead of a flat FixIt vector
>>>>>  2. Thread the change through clang, using alternative '0' in most
>>>>> places to avoid breaking existing functionality.
>>>>>  3. Add methods to add alternative FixItHints to a diagnostic
>>>>>  4. Expose this via a new libclang API. Any diagnostic rendering code
>>>>> in clang will stay the same.
>>>>> 
>>>>> There's interest from YouCompleteMe for providing an interface for
>>>>> this that lets the user pick a fix out of multiple ones, and I guess
>>>>> other IDEs can put it to great use too.
>>>>> 
>>>>> Any input on this approach or concerns?
>>>>> _______________________________________________
>>>>> cfe-dev mailing list
>>>>> cfe-dev at lists.llvm.org
>>>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
>>>> 
>>>> 
>>>> _______________________________________________
>>>> cfe-dev mailing list
>>>> cfe-dev at lists.llvm.org
>>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
>>> 
>>> 
>>> _______________________________________________
>>> cfe-dev mailing list
>>> cfe-dev at lists.llvm.org
>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
>>> 
>> 

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20160620/e8ec794f/attachment.html>


More information about the cfe-dev mailing list