[cfe-users] clang-tidy bug?

[Aaron Ballman via cfe-users]

On Thu, Oct 31, 2019 at 2:28 PM David Blaikie <dblaikie at gmail.com> wrote:
>
>
>
> On Thu, Oct 31, 2019 at 11:19 AM Aaron Ballman <aaron at aaronballman.com> wrote:
>>
>> 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.
>
>
> I wasn't thinking a way to select - but I'd expect an automated tool to, when presented with an ambiguity, decide that not doing anything was the best course of action (same as if the warning had no notes).

I agree. I've CCed Alex in case he has opinions as well.

~Aaron

>
>>
>>
>> ~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