[PATCH] Refactor: Simplify boolean expressions in lib/AST

[David Blaikie]

On Mon, Mar 23, 2015 at 11:49 PM, Daniel Jasper <djasper at google.com> wrote:

> I am actually not sure how clang-tidy should handle cases like this (where
> two checks would fix the same code in different ways) and it's probably
> something we should figure out in the long run.
>
> In this case, the else after return (yes, we do have a check for that
> although it doesn't yet offer automatic fixing) should take precedence as
> there is an explicit rule in the coding standards.
>
> Of course, that still leaves the question of whether clang-tidy should
> also simplify:
>
>   if (a)
>     return true;
>   return false;
>
> to
>
>   return a;
>

Yep - if/when the check gets to catching this (which it seems it should -
there's nothing fundamentally 'better' about this code than the other code)
then John's issue's apply again and I'd be inclined to agree with him -
though the exact nature of the suppression, I'm not sure - possibly
suppress the warning if the 'if' is an else and the prior if returns on all
paths? Dunno.


>
> On Tue, Mar 24, 2015 at 7:41 AM, David Blaikie <dblaikie at gmail.com> wrote:
>
>>
>>
>> On Mon, Mar 23, 2015 at 11:30 PM, Daniel Jasper <djasper at google.com>
>> wrote:
>>
>>> I don't feel strongly about this, and I can see some of your reasoning.
>>> However, an "if (a) return true; else return false;" is very suspect to me
>>> and I think "return a;" is more readable, independent of whether it is at
>>> the end of a chain or not.
>>>
>>> However, there is an underlying issue here. The existing code is
>>> violating LLVM's coding standard of "don't use else after return" (
>>> http://llvm.org/docs/CodingStandards.html#don-t-use-else-after-a-return).
>>> So in these chains, all of the "else"s should be removed.
>>>
>>
>> & that incidentally (sounds like more by accident than design) suppresses
>> the warning. Though, arguably, this clang-tidy check perhaps shouldn't be
>> passing judgment on that particular case and we should have an
>> else-after-return check (we probably already do) for that explicitly.
>>
>> That said, I don't much mind this check happening to provide
>> else-after-return catching in this way. (which is to say, in a round about
>> way, I agree with you Daniel) Maybe it's just a case of two different style
>> checkers happening to catch the same code for different reasons, and the
>> same potential code change addresses both of them.
>>
>>
>>>
>>> On Tue, Mar 24, 2015 at 7:17 AM, Richard <legalize at xmission.com> wrote:
>>>
>>>> Thanks for your ideas, John, I'm going to incorporate this into the
>>>> `clang-tidy` check.
>>>>
>>>>
>>>> http://reviews.llvm.org/D8532
>>>>
>>>> EMAIL PREFERENCES
>>>>   http://reviews.llvm.org/settings/panel/emailpreferences/
>>>>
>>>>
>>>>
>>>> _______________________________________________
>>>> cfe-commits mailing list
>>>> cfe-commits at cs.uiuc.edu
>>>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>>>>
>>>
>>>
>>> _______________________________________________
>>> cfe-commits mailing list
>>> cfe-commits at cs.uiuc.edu
>>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>>>
>>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20150324/21be6d45/attachment.html>