On Thu, Oct 31, 2019 at 2:28 PM David Blaikie <dblai...@gmail.com> wrote:
>
>
>
> On Thu, Oct 31, 2019 at 11:19 AM Aaron Ballman <aa...@aaronballman.com> wrote:
>>
>> On Thu, Oct 31, 2019 at 1:31 PM David Blaikie <dblai...@gmail.com> wrote:
>> >
>> >
>> >
>> > On Thu, Oct 31, 2019 at 8:45 AM Aaron Ballman <aa...@aaronballman.com>
>> > wrote:
>> >>
>> >> On Wed, Oct 30, 2019 at 9:23 PM David Blaikie <dblai...@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 <rrank...@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 <dblai...@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@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@lists.llvm.org
>> >> >>>> https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-users
_______________________________________________
cfe-users mailing list
cfe-users@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-users
