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

[Claudiu Zissulescu <claziss at gmail dot com>]

Hi Andrew,

I will repeat what Nick told me about the obvious commits (you can
trace it in binutils mailing list):

"The only exception is if the patch can be considered to be "obvious",
in which case you
can check it in without prior approval, but you must still post the
patch to the list,
and tell people that you are committing an obvious fix.  The exact
definition of obvious
in this context is a bit nebulous, but I consider it to mean not
"legally significant"[1],
not adding a new feature, and one to which any seasoned programmer
would say "oh yes,
that is obvious".

[1] http://www.gnu.org/prep/maintain/maintain.html#Legally-Significant";

I understand your patches are simple and can be considered obvious,
but as far as I can read what Nick says, we still need to go through
the reviewing process. My understanding of obvious patches are things
like spelling, typos and fixing simple warnings or so.

Anyhow, coming back to your two commits, adding the nps4xx to .cpu
directive is ok with me. The one which ignores case sensitivity, I am
in doubts, as it firstly changes the established semantic of the .cpu
pseudo-op. Then, introduces an uncertainty  how a cpu name is spelled.
Finally, it seems it is a common practice for other processors as well
to use case sensitivity match in this case. Though, I am not 100%
against this latest patch, I would like to debate the pros and cons
for such a change, although it may look trivial, the decision may
affect us years to come.

Best,
Claudiu


On Sun, Apr 17, 2016 at 10:35 PM, Andrew Burgess
<andrew.burgess@embecosm.com> wrote:
> * Claudiu Zissulescu <claziss@gmail.com> [2016-04-17 10:35:16 +0200]:
>
>> Hi,
>>
>> I am not sure if those two patches are obvious. One is changing the
>> semantic of a pseudo-op. I would expect a bit of discussion here, as
>> in our ARC backend R0 is a symbol while r0 is a register. Hence, we
>> are case sensitive. And the other one adds (doesn't fix) a new
>> feature.
>
> I apologise if I have overstepped the mark with these commits.
>
> The change in case sensitivity brings the '.cpu' directive into line
> with the -mcpu command line option.
>
> Claudiu, you make it clear in the above that you _could_ object to
> this commit, however, even after reading the above a couple of times,
> it's not clear if you are actually objecting or not.  If you don't
> like the commit then please make an objection, the commit can always
> be reverted.
>
> You are right that adding support for nps400 to the .cpu directive is
> not a bug fix, however, the code is trivial, follow the existing
> pattern of code in that function, and given that nps400 has been
> accepted I am curious under what situation you think that that part of
> the commit would be objected to in any way?
>
> Again, I apologise if anyone feels I was too forward in pushing these
> patches, I certainly did not intend to cause any offence.
>
> Thanks,
> Andrew
>
>
>
>
>>
>> Nick should be the one if I am right or not.
>>
>> Cheers,
>> Claudiu
>>
>>
>> On Sat, Apr 16, 2016 at 6:01 PM, Andrew Burgess
>> <andrew.burgess@embecosm.com> wrote:
>> > Two patches that I've pushed relating to adding support for nps400 to
>> > the ARC assembler .cpu directive.  The first patch adds support for
>> > nps400 while the second makes the .cpu directive case-insensitive.
>> >
>> > I've pushed both of these fixes as obvious.
>> >
>> > ---
>> >
>> > Andrew Burgess (2):
>> >   gas/arc: Support NPS400 in .cpu directive
>> >   gas/arc: Make .cpu directive case-insensitive
>> >
>> >  gas/ChangeLog       |  9 +++++++++
>> >  gas/config/tc-arc.c | 18 +++++++++++-------
>> >  2 files changed, 20 insertions(+), 7 deletions(-)
>> >
>> > --
>> > 2.6.4
>> >