From: Jakub Jelinek <jakub@redhat.com>
To: Martin Sebor <msebor@gmail.com>
Cc: Jeff Law <law@redhat.com>, gcc-patches@gcc.gnu.org
Subject: Re: [PATCH] Fix various minor gimple-ssa-sprintf.c issues
Date: Thu, 22 Sep 2016 07:40:00 -0000 [thread overview]
Message-ID: <20160922072411.GF7282@tucnak.redhat.com> (raw)
In-Reply-To: <7f35dddc-af39-1329-cca6-33a88955a469@gmail.com>
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
next prev parent reply other threads:[~2016-09-22 7:24 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-09-21 15:19 Jakub Jelinek
2016-09-22 4:54 ` Martin Sebor
2016-09-22 7:40 ` Jakub Jelinek [this message]
2016-09-22 8:35 ` Marek Polacek
2016-09-23 17:59 ` Martin Sebor
2016-09-22 15:29 ` Martin Sebor
2016-09-26 9:21 ` Gerald Pfeifer
2016-09-27 17:33 ` Martin Sebor
2016-09-27 17:52 ` Martin Sebor
2016-09-28 4:43 ` Jeff Law
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20160922072411.GF7282@tucnak.redhat.com \
--to=jakub@redhat.com \
--cc=gcc-patches@gcc.gnu.org \
--cc=law@redhat.com \
--cc=msebor@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).