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

[Blue Swirl]

On Mon, Jan 21, 2013 at 1:58 PM, Stefan Hajnoczi <address@hidden> wrote:
> 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.

Then we should require cleanup patches prior to patches which touch
lines with tabs. Otherwise tabs will not be removed.

>> 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