From: James Greenhalgh <james.greenhalgh@arm.com>
To: Kyrill Tkachov <kyrylo.tkachov@arm.com>
Cc: GCC Patches <gcc-patches@gcc.gnu.org>,
Marcus Shawcroft <Marcus.Shawcroft@arm.com>,
Richard Earnshaw <Richard.Earnshaw@arm.com>
Subject: Re: [PATCH][AArch64][8/14] Implement TARGET_OPTION_VALID_ATTRIBUTE_P
Date: Tue, 04 Aug 2015 09:02:00 -0000 [thread overview]
Message-ID: <20150804090243.GA10888@arm.com> (raw)
In-Reply-To: <55C07EBD.4060004@arm.com>
On Tue, Aug 04, 2015 at 09:58:37AM +0100, Kyrill Tkachov wrote:
>
> On 04/08/15 09:53, James Greenhalgh wrote:
> > On Mon, Aug 03, 2015 at 04:20:13PM +0100, Kyrill Tkachov wrote:
> >> Ok, I've removed usages of 'ret' in favor of returning when appropriate.
> >> In this last one I left the ret (but cleaned up the control flow a bit)
> >> because if the processing fails we need to clean up a bit of state before
> >> returning.
> > This is OK with the changes below fixed, or commented on as justification.
> >
> >> diff --git a/gcc/config/aarch64/aarch64-protos.h b/gcc/config/aarch64/aarch64-protos.h
> >> index fc1cec7..3a5482d 100644
> >> --- a/gcc/config/aarch64/aarch64-protos.h
> >> +++ b/gcc/config/aarch64/aarch64-protos.h
> >> @@ -376,6 +378,8 @@ extern bool aarch64_madd_needs_nop (rtx_insn *);
> >> extern void aarch64_final_prescan_insn (rtx_insn *);
> >> extern bool
> >> aarch64_expand_vec_perm_const (rtx target, rtx op0, rtx op1, rtx sel);
> >> +bool aarch64_handle_option (struct gcc_options *, struct gcc_options *,
> >> + const struct cl_decoded_option *, location_t);
> > Please try to keep this file in alphabetical order, first by return type,
> > then by function name.
>
> Ok, will do.
>
> >
> >> void aarch64_atomic_assign_expand_fenv (tree *, tree *, tree *);
> >> int aarch64_ccmp_mode_to_code (enum machine_mode mode);
> >>
> >> diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
> >> index d0d62e7..7a369fd 100644
> >> --- a/gcc/config/aarch64/aarch64.c
> >> +++ b/gcc/config/aarch64/aarch64.c
> >> +static bool
> >> +aarch64_handle_attr_arch (const char *str, const char *pragma_or_attr)
> >> +{
> >> + const struct processor *tmp_arch = NULL;
> >> + enum aarch64_parse_opt_result parse_res
> >> + = aarch64_parse_arch (str, &tmp_arch, &aarch64_isa_flags);
> >> +
> >> + if (parse_res == AARCH64_PARSE_OK)
> >> + {
> >> + gcc_assert (tmp_arch);
> >> + selected_arch = tmp_arch;
> >> + explicit_arch = selected_arch->arch;
> >> + return true;
> >> + }
> > Why not pull this in to the switch case below?
>
> I chose to keep the success case separate from error handling and reporting as it made it
> easier to find it (and it is the more interesting case in these functions). I can add a comment
> to that effect there if you'd like.
I thought that might be it. It looks unusual to me, but I don't have
strong feelings against it, so I'm happy for you to leave it as is if
that is your preference.
Thanks,
James
prev parent reply other threads:[~2015-08-04 9:02 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-07-16 15:25 Kyrill Tkachov
2015-07-21 16:07 ` James Greenhalgh
2015-07-24 8:06 ` Marcus Shawcroft
2015-07-24 10:55 ` Kyrill Tkachov
2015-08-03 10:52 ` James Greenhalgh
2015-08-03 15:20 ` Kyrill Tkachov
2015-08-04 8:53 ` James Greenhalgh
2015-08-04 8:58 ` Kyrill Tkachov
2015-08-04 9:02 ` James Greenhalgh [this message]
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=20150804090243.GA10888@arm.com \
--to=james.greenhalgh@arm.com \
--cc=Marcus.Shawcroft@arm.com \
--cc=Richard.Earnshaw@arm.com \
--cc=gcc-patches@gcc.gnu.org \
--cc=kyrylo.tkachov@arm.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).