public inbox for binutils@sourceware.org
 help / color / mirror / Atom feed
From: "Maciej W. Rozycki" <macro@imgtec.com>
To: Trevor Saunders <tbsaunde+binutils@tbsaunde.org>
Cc: <binutils@sourceware.org>
Subject: Re: [binutils-gdb] remove pointless assignment
Date: Fri, 22 Apr 2016 18:29:00 -0000	[thread overview]
Message-ID: <alpine.DEB.2.00.1604190151540.21846@tp.orcam.me.uk> (raw)
In-Reply-To: <20160416011534.2268.qmail@sourceware.org>

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

       reply	other threads:[~2016-04-22 18:29 UTC|newest]

Thread overview: 4+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
     [not found] <20160416011534.2268.qmail@sourceware.org>
2016-04-22 18:29 ` Maciej W. Rozycki [this message]
2016-04-23 14:31   ` Alan Modra
2016-04-23 18:27     ` Trevor Saunders
2016-04-25 14:04       ` Maciej W. Rozycki

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=alpine.DEB.2.00.1604190151540.21846@tp.orcam.me.uk \
    --to=macro@imgtec.com \
    --cc=binutils@sourceware.org \
    --cc=tbsaunde+binutils@tbsaunde.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).