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.
> What I would be even more grateful for is a review of the error
> prone parts like those that caused the bootstrap failure. I.e.,
> any lingering assumptions about integer sizes between the host
I must say I'm surprised you do all your computations in HOST_WIDE_INT,
rather than say in wide_int, then you could just compare against
TYPE_{MIN,MAX}_VALUE (TYPE_DOMAIN (integer_type_node)) instead of inventing
a function for that. As for the warn_* vs. flag_* stuff, it just looked
that the warn_* is tested in lots of functions invoked even for the return
length folding, so it is much harder to prove whether it works properly or
not.
Jakub
