<div dir="ltr"><div dir="ltr"><br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Thu, Oct 31, 2019 at 8:45 AM Aaron Ballman <<a href="mailto:aaron@aaronballman.com">aaron@aaronballman.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">On Wed, Oct 30, 2019 at 9:23 PM David Blaikie <<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>> wrote:<br>
><br>
> Two separate issues here<br>
><br>
> 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.<br>
><br>
> 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?<br>
<br>
I also think that's a bug. It looks like it's coming from<br>
Sema::DiagnoseEqualityWithExtraParens(). It looks like it presents<br>
both fixits, which strikes me as a bad thing to do when automatically<br>
applying fixits.<br></blockquote><div><br>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")<br><br>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?<br> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
~Aaron<br>
<br>
><br>
> On Tue, Oct 29, 2019 at 10:13 AM Robert Ankeney <<a href="mailto:rrankene@gmail.com" target="_blank">rrankene@gmail.com</a>> wrote:<br>
>><br>
>> 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<br>
>> clang-apply-replacements. Mainly I object to the suggestion+fixit to switch to an assignment. Most coding standards would disallow assignments<br>
>> in if conditionals. If anything, I would think a suggestion of "if (true == isValid)" would be more appropriate.<br>
>><br>
>> Thanks for the feedback!<br>
>> Robert<br>
>><br>
>> On Mon, Oct 28, 2019 at 2:17 PM David Blaikie <<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>> wrote:<br>
>>><br>
>>> clang-tidy in the command line you gave didn't seem to modify the file for me, did it modify the file for you?<br>
>>><br>
>>> 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.<br>
>>><br>
>>> 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.<br>
>>><br>
>>> - Dave<br>
>>><br>
>>> On Sun, Oct 27, 2019 at 11:32 AM Robert Ankeney via cfe-users <<a href="mailto:cfe-users@lists.llvm.org" target="_blank">cfe-users@lists.llvm.org</a>> wrote:<br>
>>>><br>
>>>> For the following code (wrong.cpp):<br>
>>>><br>
>>>> bool check(bool isValid)<br>
>>>> {<br>
>>>> bool retVal = false;<br>
>>>><br>
>>>> if (( isValid == true ))<br>
>>>> {<br>
>>>> retVal = true;<br>
>>>> }<br>
>>>><br>
>>>> return retVal;<br>
>>>> }<br>
>>>><br>
>>>> when I run:<br>
>>>> clang-tidy -checks=modernize-use-default-member-init wrong.cpp<br>
>>>><br>
>>>> I get:<br>
>>>> 4 warnings and 1 error generated.<br>
>>>> Error while processing /llvm/match/ctBad/wrong.cpp.<br>
>>>> /llvm/match/ctBad/wrong.cpp:5:19: error: equality comparison with extraneous parentheses [clang-diagnostic-parentheses-equality]<br>
>>>> if (( isValid == true ))<br>
>>>> ~ ^~ ~<br>
>>>> =<br>
>>>> /llvm/match/ctBad/wrong.cpp:5:19: note: remove extraneous parentheses around the comparison to silence this warning<br>
>>>> /llvm/match/ctBad/wrong.cpp:5:19: note: use '=' to turn this equality comparison into an assignment<br>
>>>><br>
>>>> Note it turns the if into:<br>
>>>> if ( isValid = true )<br>
>>>><br>
>>>> 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?<br>
>>>><br>
>>>> Thanks,<br>
>>>> Robert<br>
>>>><br>
>>>> _______________________________________________<br>
>>>> cfe-users mailing list<br>
>>>> <a href="mailto:cfe-users@lists.llvm.org" target="_blank">cfe-users@lists.llvm.org</a><br>
>>>> <a href="https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-users" rel="noreferrer" target="_blank">https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-users</a><br>
</blockquote></div></div>