This is the mail archive of the
binutils@sourceware.org
mailing list for the binutils project.
Re: [PATCH 0/2] [PUSHED/OBV] gas/arc: Add nps400 support to .cpu directive
- From: Nick Clifton <nickc at redhat dot com>
- To: Andrew Burgess <andrew dot burgess at embecosm dot com>, Claudiu Zissulescu <claziss at gmail dot com>
- Cc: Binutils <binutils at sourceware dot org>, Claudiu Zissulescu <Claudiu dot Zissulescu at synopsys dot com>, Cupertino Miranda <Cupertino dot Miranda at synopsys dot com>
- Date: Mon, 18 Apr 2016 12:07:44 +0100
- Subject: Re: [PATCH 0/2] [PUSHED/OBV] gas/arc: Add nps400 support to .cpu directive
- Authentication-results: sourceware.org; auth=none
- References: <cover dot 1460822027 dot git dot andrew dot burgess at embecosm dot com> <CAL0iMy1eDXHNYZS_B=zNvV+FMnp09yPHo2gxYLTiHEYgk=LG+A at mail dot gmail dot com> <20160417203517 dot GC6589 at embecosm dot com> <CAL0iMy0iDrUA_2U1WDnsym1z6a-wATDWyY932Mr5maK5dt_dPg at mail dot gmail dot com> <20160417223058 dot GD6589 at embecosm dot com>
Hi Andrew,
> I have taken the liberty of leaving this commit in the tree. I'll
> keep my fingers crossed that a global maintainer doesn't object :-)
Actually I do object, sorry. :-{
There are problems with both of the patches:
[PATCH 1/2] gas/arc: Support NPS400 in .cpu directive
The problem here is not the patch itself, it is OK, but rather
that it does not go far enough. The patch adds support for the
NPS400 to the .cpu directive, but it does not update the
documentation in gas/doc/c-arc.texi detailing the acceptable
arguments to the .cpu directive.
[A patch to update the documentation as indicated is pre-approved].
Further I would suggest that this code can be improved. Since
there already is a table of known ARC architecture variants in tc-arc.c
(cpu_types[], line 420), it would make sense to change the code in
arc_option() to scan this table. That way when a new architecture is
added in the future the programmer will not have to remember to
update the arc_option() function as well.
Additionally the md_show_usage() function could be extended to list
exactly which architectures are supported by the -mcpu= command line
option; again by scanning the cpu_types table. Other targets already
do this sort of thing.
[The patch can stay in, but please do consider the above advice].
[PATCH 2/2] gas/arc: Make .cpu directive case-insensitive
This is definitely not "obvious" because you are changing the
functionality of the assembler. It might be trivial, but it
certainly is not obvious.
As Claudiu pointed out removing case-sensitivity does have its
problems. If the two of you can agree that it is the right thing
to do, then I would be OK with the patch - although you would also
need to update the documentation in gas/doc/c-arc.texi to explicitly
state that the parameter to the .cpu directive is case insensitive.
So, first of all, please revert this patch. Then if you still feel
that case insensitivity is important, please present your arguments
on this list. Assuming that agreement can be reached, please submit
a revised patch for review afterwards.
Cheers
Nick