[cfe-users] clang-tidy bug?

[Aaron Ballman via cfe-users]

On Thu, Oct 31, 2019 at 1:31 PM David Blaikie <dblaikie at gmail.com> wrote:
>
>
>
> On Thu, Oct 31, 2019 at 8:45 AM Aaron Ballman <aaron at aaronballman.com> wrote:
>>
>> On Wed, Oct 30, 2019 at 9:23 PM David Blaikie <dblaikie at gmail.com> wrote:
>> >
>> > Two separate issues here
>> >
>> > 1) the fixit hint, as one of a set of alternatives, isn't likely to be removed/changed - the (albeit quirky) convention of using extra () to indicate an intentional assignment in a condition has been around for a while. So if you use the extra parens without writing an assignment, the compiler's going to suggest you resolve this "conflict" with the style - either you didn't intend the extra (), or you intended to use assignment. Those are the two alternative suggestions being made.
>> >
>> > 2) automatically applying one fixit hint of several ambiguous ones seems like a bug to me - Aaron - do you know anything about this? Is this accurate/intended/etc?
>>
>> I also think that's a bug. It looks like it's coming from
>> Sema::DiagnoseEqualityWithExtraParens(). It looks like it presents
>> both fixits, which strikes me as a bad thing to do when automatically
>> applying fixits.
>
>
> These fixits should be attached to notes, though, right? & Clang produces all sorts of fixits on notes that are not semantics-preserving, I think? ("oh, I think you meant to write this other thing")

Yes, they're both attached to notes but the notes point to the same
location as the error.

> My understanding is that the fixits are correct (in the clang diagnostic experience - "here's a warning, here are some ideas of what you might've intended that would address the warning") - but it seems incorrect to automatically apply especially ambiguous suggesitons like this. How would clang-tidy choose between the two alternatives?

I don't think it has a way to select between alternatives; I don't
think that was a use case we had envisioned for automatically applying
fix-its.

~Aaron

>
>>
>>
>> ~Aaron
>>
>> >
>> > On Tue, Oct 29, 2019 at 10:13 AM Robert Ankeney <rrankene at gmail.com> wrote:
>> >>
>> >> This was just a example of what I ran into when I used run-clang-tidy.py across a compilation database with a -export-fixes=fix.yaml and then ra
>> >>  clang-apply-replacements. Mainly I object to the suggestion+fixit to switch to an assignment. Most coding standards would disallow assignments
>> >> in if conditionals. If anything, I would think a suggestion of "if (true == isValid)" would be more appropriate.
>> >>
>> >> Thanks for the feedback!
>> >> Robert
>> >>
>> >> On Mon, Oct 28, 2019 at 2:17 PM David Blaikie <dblaikie at gmail.com> wrote:
>> >>>
>> >>> clang-tidy in the command line you gave didn't seem to modify the file for me, did it modify the file for you?
>> >>>
>> >>> Are you objecting to the suggestion, or that it was automatically applied? I would think it'd be a bug to apply any fixit/hint if there are multiple possible suggestions.
>> >>>
>> >>> But the existence of the suggestion (without the application of it) to the user seems right to me. The use of extra () to suppress the assignment-in-conditional warning (-Wparentheses) has been around for quite a while, so it's possible that the user intended assignment rather than comparison when they added the extra parentheses.
>> >>>
>> >>> - Dave
>> >>>
>> >>> On Sun, Oct 27, 2019 at 11:32 AM Robert Ankeney via cfe-users <cfe-users at lists.llvm.org> wrote:
>> >>>>
>> >>>> For the following code (wrong.cpp):
>> >>>>
>> >>>> bool check(bool isValid)
>> >>>> {
>> >>>>     bool retVal = false;
>> >>>>
>> >>>>     if (( isValid == true ))
>> >>>>     {
>> >>>>         retVal = true;
>> >>>>     }
>> >>>>
>> >>>>     return retVal;
>> >>>> }
>> >>>>
>> >>>> when I run:
>> >>>>     clang-tidy -checks=modernize-use-default-member-init wrong.cpp
>> >>>>
>> >>>> I get:
>> >>>> 4 warnings and 1 error generated.
>> >>>> Error while processing /llvm/match/ctBad/wrong.cpp.
>> >>>> /llvm/match/ctBad/wrong.cpp:5:19: error: equality comparison with extraneous parentheses [clang-diagnostic-parentheses-equality]
>> >>>>     if (( isValid == true ))
>> >>>>         ~         ^~      ~
>> >>>>                   =
>> >>>> /llvm/match/ctBad/wrong.cpp:5:19: note: remove extraneous parentheses around the comparison to silence this warning
>> >>>> /llvm/match/ctBad/wrong.cpp:5:19: note: use '=' to turn this equality comparison into an assignment
>> >>>>
>> >>>> Note it turns the if into:
>> >>>>     if ( isValid = true )
>> >>>>
>> >>>> Seems like a very bad idea. Removing the redundant parentheses seems fine, but changing the comparison to an assignment does not. Is this a bug?
>> >>>>
>> >>>> Thanks,
>> >>>> Robert
>> >>>>
>> >>>> _______________________________________________
>> >>>> cfe-users mailing list
>> >>>> cfe-users at lists.llvm.org
>> >>>> https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-users