Re: [PATCH 0/2] [PUSHED/OBV] gas/arc: Add nps400 support to .cpu directive

[Nick Clifton <nickc at redhat 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