* Re: [binutils-gdb] remove pointless assignment
[not found] <20160416011534.2268.qmail@sourceware.org>
@ 2016-04-22 18:29 ` Maciej W. Rozycki
2016-04-23 14:31 ` Alan Modra
0 siblings, 1 reply; 4+ messages in thread
From: Maciej W. Rozycki @ 2016-04-22 18:29 UTC (permalink / raw)
To: Trevor Saunders; +Cc: binutils
Trevor,
Why did you commit this change without a review? I saw Alan made some
suggestions on top of your original submission, but he didn't say a change
with these updates was preapproved, and neither your change falls into the
obviously correct category. Also in any case you need to post to the
mailing list the final version committed.
I have a few notes for you for the future which I would make in the
review process if you gave me the chance, see below.
> commit 92fce9bd7a4d5732fe9db05b7ebaef4ab858e69a
> Author: Trevor Saunders <tbsaunde+binutils@tbsaunde.org>
> Date: Wed Apr 13 05:03:22 2016 -0400
>
> remove pointless assignment
Please try to make your patch headings more descriptive, with a clean-up
like this at least identifying the place where the change is made, so that
commands like `git shortlog' or `git log --abbrev-commit --pretty=oneline'
give useful results. Here e.g.:
MIPS/GAS: Remove pointless assignment in `md_begin'
would do -- with the orignal change that is, as the final one went far
beyond that.
> Presumably this was supposed to be regname[sizeof (regname) - 1] but was typoed
> to regname[sizeof (rename) - 1]. However that should be unnecessary because
> sprintf should null terminate. As is this assignment is invalid ISO C because
> rename refers to the function rename (), and sizeof on functions is undefined.
> In GNU C C the size of functions is 1 so the expression is the same as
> regname[0]. The following call to sprintf () clearly will over right that, so
> the statement either has no effect or is invalid. Given that it seems safe to
> just remove it. While we are there correct the size of regname, and switch
> from snprintf to sprintf since we know the exact length of the result.
Please try to keep lines of the commit description at most 74 characters
wide as with ChangeLog entries, bearing in mind that commands like `git
show' will indent it by 4 characters. This is to make them render in a
readable way on a standard 80-character terminal.
Your message shows here like this:
Presumably this was supposed to be regname[sizeof (regname) - 1] but was typ
oed
to regname[sizeof (rename) - 1]. However that should be unnecessary because
sprintf should null terminate. As is this assignment is invalid ISO C becau
se
rename refers to the function rename (), and sizeof on functions is undefine
d.
[...]
which disturbs reading.
> gas/ChangeLog:
>
> 2016-04-15 Trevor Saunders <tbsaunde+binutils@tbsaunde.org>
>
> * config/tc-mips.c (md_begin): Remove useless assignment.
Your ChangeLog entry does not correspond to the actual change made.
Please make sure it remains consistent as you make updates.
Thank you for your cooperation and your contribution, and happy hacking!
Maciej
^ permalink raw reply [flat|nested] 4+ messages in thread