public inbox for binutils@sourceware.org
 help / color / mirror / Atom feed
From: James E Wilson <wilson@specifixinc.com>
To: Jan Beulich <JBeulich@novell.com>
Cc: binutils@sources.redhat.com
Subject: Re: [PATCH] ia64: unwind directive handling
Date: Fri, 01 Jul 2005 03:10:00 -0000	[thread overview]
Message-ID: <1120187407.32003.100.camel@aretha.corp.specifixinc.com> (raw)
In-Reply-To: <42BFF1C3020000780001D842@emea1-mh.id2.novell.com>

On Mon, 2005-06-27 at 03:32, Jan Beulich wrote:
> Here's the updated patch, with (I believe) all issues addressed (except for where I gave reasons for why things were done the way they are in an earlier reply). Additionally to that there are four more new warnings (noting inconsistencies between .prologue and later .save-s).

Sorry about the delays.  I'm hoping to be more responsive now that the
GCC Summit is over.  It messed up my schedule the last few weeks.

I have no problem with any of the answers you've given, or the
subsequent changes you made.

About the default in the middle of the switch statement issue, I missed
the fact that there was a missing break.  There is a relatively common
convention to add a /* FALLTHOUGH */ marker in this case.  This makes it
obvious that the missing break is intentional, and not a bug.  It also
makes it obvious that the case labels can't be reordered without
breaking the code.  Anyways, in this case, I think the rewrite is
better.  Your original code was too clever for its own good.  This
invariably leads to problems, as at some point in the future someone you
don't know will fail to understand how clever the code is, assume it is
broken, and then proceed to fix it.  Clear code is better than clever
code, even if it is a few lines longer.  And yes, gotos should be
avoided.

I said I was going to do some testing with the first version of the
patch.  glibc built OK.  I found two minor typos in the linux kernel,
spurious characters at the end of an unwind directive that were now
caught whereas they used to be ignored.  Both have been reported, and I
think both have already been fixed.  In any case, there is nothing to
worry about here.  I may have forgetten to do the gcc bootstrap.  I
don't recall.

The patch is OK.
-- 
Jim Wilson, GNU Tools Support, http://www.SpecifixInc.com


  reply	other threads:[~2005-07-01  3:10 UTC|newest]

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2005-06-27 10:31 Jan Beulich
2005-07-01  3:10 ` James E Wilson [this message]
  -- strict thread matches above, loose matches on Subject: below --
2005-06-15 15:31 Jan Beulich
2005-06-13  7:07 Jan Beulich
2005-06-10 21:52 Cary Coutant
2005-06-10 13:21 Jan Beulich
2005-05-23 10:19 Jan Beulich
2005-06-10  0:07 ` James E Wilson

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=1120187407.32003.100.camel@aretha.corp.specifixinc.com \
    --to=wilson@specifixinc.com \
    --cc=JBeulich@novell.com \
    --cc=binutils@sources.redhat.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).