[PATCH 0/2] [PUSHED/OBV] gas/arc: Add nps400 support to .cpu directive
Nick Clifton
nickc@redhat.com
Mon Apr 18 11:07:00 GMT 2016
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
More information about the Binutils
mailing list