Re: [Qemu-devel] [PATCH] readline: avoid memcpy() of overlapping regions

[Stefan Hajnoczi]

On Sat, Jan 19, 2013 at 09:04:57AM +0000, Blue Swirl wrote:
> On Fri, Jan 18, 2013 at 11:04 AM, Stefan Hajnoczi <address@hidden> wrote:
> > On Thu, Jan 17, 2013 at 08:13:38PM +0000, Blue Swirl wrote:
> >> On Mon, Jan 14, 2013 at 9:09 AM, Stefan Hajnoczi <address@hidden> wrote:
> >> > On Sat, Jan 12, 2013 at 10:46:18AM +0000, Blue Swirl wrote:
> >> >> On Thu, Jan 10, 2013 at 12:43 PM, Stefan Hajnoczi <address@hidden> 
> >> >> wrote:
> >> >> > On Wed, Jan 09, 2013 at 08:43:51PM +0000, Blue Swirl wrote:
> >> >> >> On Tue, Jan 8, 2013 at 9:03 AM, Stefan Hajnoczi <address@hidden> 
> >> >> >> wrote:
> >> >> >> > On Mon, Jan 07, 2013 at 03:38:39PM -0500, Nickolai Zeldovich wrote:
> >> >> >> >> memcpy() for overlapping regions is undefined behavior; use 
> >> >> >> >> memmove()
> >> >> >> >> instead in readline_hist_add().
> >> >> >> >>
> >> >> >> >> Signed-off-by: Nickolai Zeldovich <address@hidden>
> >> >> >> >> ---
> >> >> >> >>  readline.c |    4 ++--
> >> >> >> >>  1 file changed, 2 insertions(+), 2 deletions(-)
> >> >> >> >
> >> >> >> > I made a slight modification: keep the tab characters since the
> >> >> >> > surrounding code still uses them.
> >> >> >>
> >> >> >> I think tabs should be fixed whenever possible, otherwise we may 
> >> >> >> never
> >> >> >> get them converted.
> >> >> >
> >> >> > Not in a one-line patch when the surrounding lines still use them.  It
> >> >> > creates a mess.
> >> >>
> >> >> Only if the reader messes with the tab width settings (and in that
> >> >> case they deserve what they get and they are probably also used to
> >> >> this), otherwise a line with tabs converted to spaces looks exactly
> >> >> the same.
> >> >
> >> > You are oversimplifying how tab widths work.  The author and reader's
> >> > tab width must match in order for displayed text to appear correctly.
> >>
> >> Exactly. The default tab width is 8 in all tools and it takes some
> >> effort to adjust the tab settings in each tool. For example, how do
> >> you change it in less, xterm or cmd.exe?
> >>
> >> It is unreasonable and arrogant for an author to assume any other
> >> setting used by the reader for tab width, even if this was declared
> >> for example at the start of the file.
> >>
> >> Perhaps an analogy could be a compressing or encrypting preprocessor
> >> for the white space in the code. Without the right tool and correct
> >> settings, the reader would not see the white space in the code
> >> correctly.
> >>
> >> > Tell me what you consider the "correct" tab width for readers and I'll
> >> > find a piece of QEMU code that was authored for a different tab width
> >> > :).
> >>
> >> 8.
> >>
> >> >
> >> > In other words, it's a mess and best not to perturb it further.
> >>
> >> No, those messes should be cleaned up, much like braces.
> >
> > Agreed but in cleanup patches or when touching the whole function.  Not
> > one line at a time because then we have an even bigger mess.
> 
> So you are advocating cleanup patches which only adjust formatting?

Yes.  Also, cleaning up the entire function when making significant
changes is good.

> How about mass conversion then? Anthony has rejected the similar
> approach for braces because of the loss of git blame info.

There are drawbacks to mass converting code that isn't being otherwise
touched.  It makes git-blame(1) harder to use and the patches still
require code review from the community.

Best to convert files that are under active development, where it's
worth reformatting and we rebuild commit history quickly.

For files that are rarely/never touched, the drawback can be bigger than
the advantage.

Stefan