[cfe-users] clang-tidy bug?

[David Blaikie via cfe-users]

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")

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?


>
> ~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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-users/attachments/20191031/16840098/attachment.html>