Re: [PATCH v2 1/1] objdump, as: Add callx support for BPF CPU v1

[Nick Clifton <nickc@redhat.com>]

Hi Will,

>> Just to be clear: your sign off here does mean that you agree to the
>> Developer Certificate of Origin (v1.1) right ?  [I am being paranoid...:-]
>>
> 
> Yes, 100%! I appreciate you checking but I specifically included the
> sign off as my agreement. If there is a better way to positively
> signal that intention, please let me know!

No, that is the right way.  I was only checking, because as a new contributor
(to the GNU Binutils at least) I wanted to be certain that you were aware of
the significance of the sign-off.


>> This frag struck me as strange since it appears that the first change is to
>> replace a line with itself.  Looking a bit closer I saw that you are removing
>> some unnecessary spaces at the end of the line.  Which is good and not a
>> problem.  It just looked odd when I was reviewing the patch.
> 
> I was hesitant to include this piece in the PR because I know that
> whitespace patches are always a touchy subject

Really ?  Personally I have no problem with whitespace cleanup patches.
My only real worry is when they appear as part of a patch that is doing
something else.  Generally speaking it is always best to keep patches
as simple as possible, so a patch that changes code, cleans up whitespace
and corrects spelling mistakes all in one is going to be frowned upon (by
me at least) whereas as three separate patches would be welcome.


> Here is my offer (hinted at above): There are several parts of this
> particular file that appear out of the ordinary:
> 
> 1. Inconsistent tab/space usage
> 2. The presence of magic values
> 3. Spaces at the end of lines
> 
> And so on.
> 
> If I were to take a pass at "cleaning up" the code in this file, would
> you be interested in having a PR?

Yes.

Sorry - I was confused when I first read this as "PR" to me normally
means "Problem Report", as in a bug filed in the binutils bugzilla system.
These are assigned PR numbers and can often be seen referred to in comments
in the code and the changelogs.

I assume however that you mean "Pull Request" here, which makes sense in
context, except that the binutils project being old and stuffy we normally
go by patch submissions rather than pull requests...


One other thing I should also mention.  Whenever you do submit a patch it
always helps if you mention how it was tested.  Noting that it was tested
with a binutils build configured for bpf-elf and that there were no
regressions in the gas, binutils or ld testsuites is reassuring and helps
to make the job of reviewing the patch easier.  This applies even to code
cleanup patches, where of course no test regressions are expected, but you
never know...


> As I said, I really appreciate your
> openness to my contribution and I love working with FOSS. In other
> words, I would really enjoy contributing. That said, I don't want to
> step in and intrude on someone else's territory. Just let me know!

Oh no, please do step in.  No-one has a "its all mine" territory.  We do
have contributors who have volunteered to act as maintainers for certain
architectures and/or parts of the binutils, and they will often be the
ones who review patch submissions in their areas.  But everyone is welcome
to submit patches to any and all parts of the binutils.

Cheers  Nick