Re: [PATCH] Fix various minor gimple-ssa-sprintf.c issues

[Martin Sebor <msebor@gmail.com>]

On 09/22/2016 01:46 AM, Marek Polacek wrote:
> On Thu, Sep 22, 2016 at 09:24:11AM +0200, Jakub Jelinek wrote:
>> On Wed, Sep 21, 2016 at 06:38:54PM -0600, Martin Sebor wrote:
>>> On 09/21/2016 09:09 AM, Jakub Jelinek wrote:
>>>> When looking at PR77676, I've noticed various small formatting etc.
>>>> issues, like not using is_gimple_* APIs where we have them, not using
>>>> gimple_call_builtin_p/gimple_call_fndecl (this one actually can show up,
>>>> if e.g. uses the builtin with incorrect arguments (fewer, different
>>>> types etc.)), one pasto, 2 spaces in comments instead of 1 in the middle
>>>> of sentences.  And, lastly 0 < var is very unusual ordering of the
>>>> comparison operands, while we have a couple of such cases in the sources,
>>>> usually it is when using 0 < var && var <= someotherconst, while
>>>> var > 0 is used hundred times more often.
>>>
>>> Thanks for correcting the uses of the gimple APIs!  I appreciate
>>> your fixing the various typos as well, but I see no value in
>>> changing the order of operands in inequality expressions or in
>>> moving code around for no apparent reason.  However, I won't
>>
>> The moving of code around is in just one spot, and it has a reason -
>> consistency.  After the move, each non-_chk builtin is followed by its _chk
>> counterpart, before that the order has been random.
>> Another possible ordering that makes sense is putting all the non-_chk
>> builtins first and then in the same order all their _chk counterparts.
>>
>> The reason why I wrote the patch has been that when skimming the code I've
>> noticed the missing is_* calls, then when looking for that issue discovered
>> something different etc.  The var > 0 vs. 0 < var is just something that
>> caught my eye when looking around, I don't feel too strongly about it, it
>> just looked weird and unexpected.  There are > 50 optimize > 0 preexisting
>> checks elsewhere, and even far more just optimize, but none 0 < optimize.
>
> I find those 0 < var confusing and hard to read.  While I know that some
> people prefer 0 == var (0 is not an lvalue so it catches mistakes like
> var = 0 instead of var == 0), I don't see why 0 < optimized would ever be
> preferred.

I don't have a preference for one or the other.  It's just how
I wrote the code.  Over the years of working on the C++ standard
library I trained myself to use the less than expression in
preference to any of the others because C++ algorithms had to
(or we wanted them to, I don't remember which anymore) work with 
user-defined types that only defined that one relational operator
and not any of the other forms.

Martin