Patchwork readline: avoid memcpy() of overlapping regions

login
register
mail settings
Submitter Nickolai Zeldovich
Date Jan. 7, 2013, 8:38 p.m.
Message ID <1357591119-40326-1-git-send-email-nickolai@csail.mit.edu>
Download mbox | patch
Permalink /patch/210224/
State New
Headers show

Comments

Nickolai Zeldovich - Jan. 7, 2013, 8:38 p.m.
memcpy() for overlapping regions is undefined behavior; use memmove()
instead in readline_hist_add().

Signed-off-by: Nickolai Zeldovich <nickolai@csail.mit.edu>
---
 readline.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
Richard Henderson - Jan. 7, 2013, 9:36 p.m.
On 01/07/2013 12:38 PM, Nickolai Zeldovich wrote:
> memcpy() for overlapping regions is undefined behavior; use memmove()
> instead in readline_hist_add().
> 
> Signed-off-by: Nickolai Zeldovich <nickolai@csail.mit.edu>
> ---
>  readline.c |    4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

This probably should go in via trivial...

Reviewed-by: Richard Henderson <rth@twiddle.net>


r~
Stefan Hajnoczi - Jan. 8, 2013, 9:03 a.m.
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 <nickolai@csail.mit.edu>
> ---
>  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.

Thanks, applied to the trivial patches tree:
https://github.com/stefanha/qemu/commits/trivial-patches

Stefan
Blue Swirl - Jan. 9, 2013, 8:43 p.m.
On Tue, Jan 8, 2013 at 9:03 AM, Stefan Hajnoczi <stefanha@gmail.com> 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 <nickolai@csail.mit.edu>
>> ---
>>  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.

>
> Thanks, applied to the trivial patches tree:
> https://github.com/stefanha/qemu/commits/trivial-patches
>
> Stefan
>
Stefan Hajnoczi - Jan. 10, 2013, 12:43 p.m.
On Wed, Jan 09, 2013 at 08:43:51PM +0000, Blue Swirl wrote:
> On Tue, Jan 8, 2013 at 9:03 AM, Stefan Hajnoczi <stefanha@gmail.com> 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 <nickolai@csail.mit.edu>
> >> ---
> >>  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.

Stefan
Blue Swirl - Jan. 12, 2013, 10:46 a.m.
On Thu, Jan 10, 2013 at 12:43 PM, Stefan Hajnoczi <stefanha@gmail.com> 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 <stefanha@gmail.com> 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 <nickolai@csail.mit.edu>
>> >> ---
>> >>  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.

I think this is identical case to braces. If we don't allow mass
conversion, conversion as a side effect is the only way and then it
should be always followed.

>
> Stefan
Stefan Hajnoczi - Jan. 14, 2013, 9:09 a.m.
On Sat, Jan 12, 2013 at 10:46:18AM +0000, Blue Swirl wrote:
> On Thu, Jan 10, 2013 at 12:43 PM, Stefan Hajnoczi <stefanha@gmail.com> 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 <stefanha@gmail.com> 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 <nickolai@csail.mit.edu>
> >> >> ---
> >> >>  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.

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
:).

In other words, it's a mess and best not to perturb it further.

Stefan
Blue Swirl - Jan. 17, 2013, 8:13 p.m.
On Mon, Jan 14, 2013 at 9:09 AM, Stefan Hajnoczi <stefanha@gmail.com> 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 <stefanha@gmail.com> 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 <stefanha@gmail.com> 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 <nickolai@csail.mit.edu>
>> >> >> ---
>> >> >>  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.

>
> Stefan
Stefan Hajnoczi - Jan. 18, 2013, 11:04 a.m.
On Thu, Jan 17, 2013 at 08:13:38PM +0000, Blue Swirl wrote:
> On Mon, Jan 14, 2013 at 9:09 AM, Stefan Hajnoczi <stefanha@gmail.com> 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 <stefanha@gmail.com> 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 <stefanha@gmail.com> 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 <nickolai@csail.mit.edu>
> >> >> >> ---
> >> >> >>  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.

Stefan
Blue Swirl - Jan. 19, 2013, 9:04 a.m.
On Fri, Jan 18, 2013 at 11:04 AM, Stefan Hajnoczi <stefanha@gmail.com> 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 <stefanha@gmail.com> 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 <stefanha@gmail.com> 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 <stefanha@gmail.com> 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 <nickolai@csail.mit.edu>
>> >> >> >> ---
>> >> >> >>  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?
How about mass conversion then? Anthony has rejected the similar
approach for braces because of the loss of git blame info.

>
> Stefan
Stefan Hajnoczi - Jan. 21, 2013, 1:58 p.m.
On Sat, Jan 19, 2013 at 09:04:57AM +0000, Blue Swirl wrote:
> On Fri, Jan 18, 2013 at 11:04 AM, Stefan Hajnoczi <stefanha@gmail.com> 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 <stefanha@gmail.com> 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 <stefanha@gmail.com> 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 <stefanha@gmail.com> 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 <nickolai@csail.mit.edu>
> >> >> >> >> ---
> >> >> >> >>  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
Blue Swirl - Jan. 21, 2013, 8:06 p.m.
On Mon, Jan 21, 2013 at 1:58 PM, Stefan Hajnoczi <stefanha@gmail.com> 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 <stefanha@gmail.com> 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 <stefanha@gmail.com> 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 <stefanha@gmail.com> 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 <stefanha@gmail.com> 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 <nickolai@csail.mit.edu>
>> >> >> >> >> ---
>> >> >> >> >>  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
Andreas Färber - Jan. 24, 2013, 5:17 p.m.
Am 17.01.2013 21:13, schrieb Blue Swirl:
> On Mon, Jan 14, 2013 at 9:09 AM, Stefan Hajnoczi <stefanha@gmail.com> wrote:
>> 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.

So FWIW one exception is target-cris/helper.c, which seems to use 4. :)
Many Windows and Eclipse-based editors use 4.

My personal opinion is that tabs don't have any fixed width and, when
used properly, that works fairly well (i.e., indent the block with tabs
and do any parenthesis alignment etc. with blanks from block level).

Andreas
Blue Swirl - Jan. 25, 2013, 6:07 p.m.
On Thu, Jan 24, 2013 at 5:17 PM, Andreas Färber <afaerber@suse.de> wrote:
> Am 17.01.2013 21:13, schrieb Blue Swirl:
>> On Mon, Jan 14, 2013 at 9:09 AM, Stefan Hajnoczi <stefanha@gmail.com> wrote:
>>> 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.
>
> So FWIW one exception is target-cris/helper.c, which seems to use 4. :)

I don't think so:
	miss = cris_mmu_translate(&res, env, address & TARGET_PAGE_MASK,
				  rw, mmu_idx, 0);
The first line has one tab (with tab equal to 8 spaces, column 8) and
the second, four tabs plus two spaces to reach column 34 (4 * 8 + 2)
so 'rw' is nicely aligned to just after '(' in the first line. If we
instead assumed a tab size of 4, the first line would have indent of 4
spaces but the second 4 * 4 + 2 = 10 which would mess the indentation.

> Many Windows and Eclipse-based editors use 4.
>
> My personal opinion is that tabs don't have any fixed width and, when
> used properly, that works fairly well (i.e., indent the block with tabs
> and do any parenthesis alignment etc. with blanks from block level).

If the indentation did not make any assumptions about the tab
settings, that could actually work. But using the above example, the
second line should use only one tab and 26 spaces, then both lines
would be aligned with any tab settings. Does anyone use tabs like
this?

>
> Andreas
>
> --
> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
Andreas Färber - Jan. 25, 2013, 6:30 p.m.
Am 25.01.2013 19:07, schrieb Blue Swirl:
> On Thu, Jan 24, 2013 at 5:17 PM, Andreas Färber <afaerber@suse.de> wrote:
>> Am 17.01.2013 21:13, schrieb Blue Swirl:
>>> On Mon, Jan 14, 2013 at 9:09 AM, Stefan Hajnoczi <stefanha@gmail.com> wrote:
>>>> 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.
>>
>> So FWIW one exception is target-cris/helper.c, which seems to use 4. :)
> 
> I don't think so:
> 	miss = cris_mmu_translate(&res, env, address & TARGET_PAGE_MASK,
> 				  rw, mmu_idx, 0);
> The first line has one tab (with tab equal to 8 spaces, column 8) and
> the second, four tabs plus two spaces to reach column 34 (4 * 8 + 2)
> so 'rw' is nicely aligned to just after '(' in the first line. If we
> instead assumed a tab size of 4, the first line would have indent of 4
> spaces but the second 4 * 4 + 2 = 10 which would mess the indentation.

There's other lines where it's not nicely aligned at 8 chars and unlike
other files it did not use 4 spaces for one indent and then one tab for
the next. Anyway, patch to fix this is out for Edgar to apply or not to.

http://patchwork.ozlabs.org/patch/215814/

>> My personal opinion is that tabs don't have any fixed width and, when
>> used properly, that works fairly well (i.e., indent the block with tabs
>> and do any parenthesis alignment etc. with blanks from block level).
> 
> If the indentation did not make any assumptions about the tab
> settings, that could actually work. But using the above example, the
> second line should use only one tab and 26 spaces, then both lines
> would be aligned with any tab settings. Does anyone use tabs like
> this?

I did so in earlier times, using Visual Studio, Eclipse or Xcode
configured to show me the whitespace chars. Any automatic reformatting
breaks it though...
But I'm not suggesting QEMU to adopt it. I just disagree on any "tab is
meant to be N chars wide" hypothesis of the day. :)

Andreas
Markus Armbruster - Jan. 25, 2013, 8:21 p.m.
Blue Swirl <blauwirbel@gmail.com> writes:

> On Thu, Jan 24, 2013 at 5:17 PM, Andreas Färber <afaerber@suse.de> wrote:
>> Am 17.01.2013 21:13, schrieb Blue Swirl:
>>> On Mon, Jan 14, 2013 at 9:09 AM, Stefan Hajnoczi <stefanha@gmail.com> wrote:
>>>> 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.
>>
>> So FWIW one exception is target-cris/helper.c, which seems to use 4. :)
>
> I don't think so:
> 	miss = cris_mmu_translate(&res, env, address & TARGET_PAGE_MASK,
> 				  rw, mmu_idx, 0);
> The first line has one tab (with tab equal to 8 spaces, column 8) and
> the second, four tabs plus two spaces to reach column 34 (4 * 8 + 2)
> so 'rw' is nicely aligned to just after '(' in the first line. If we
> instead assumed a tab size of 4, the first line would have indent of 4
> spaces but the second 4 * 4 + 2 = 10 which would mess the indentation.
>
>> Many Windows and Eclipse-based editors use 4.
>>
>> My personal opinion is that tabs don't have any fixed width and, when
>> used properly, that works fairly well (i.e., indent the block with tabs
>> and do any parenthesis alignment etc. with blanks from block level).
>
> If the indentation did not make any assumptions about the tab
> settings, that could actually work. But using the above example, the
> second line should use only one tab and 26 spaces, then both lines
> would be aligned with any tab settings. Does anyone use tabs like
> this?

Wrong question.  The correct question is "Does everybody[*] use tabs
like this?"


[*] Everybody hacking on a particular body of code.  For a personal
project, that may be just that one person.

Patch

diff --git a/readline.c b/readline.c
index 5fc9643..aeccc7b 100644
--- a/readline.c
+++ b/readline.c
@@ -248,8 +248,8 @@  static void readline_hist_add(ReadLineState *rs, const char *cmdline)
     if (idx == READLINE_MAX_CMDS) {
 	/* Need to get one free slot */
 	free(rs->history[0]);
-	memcpy(rs->history, &rs->history[1],
-	       (READLINE_MAX_CMDS - 1) * sizeof(char *));
+        memmove(rs->history, &rs->history[1],
+                (READLINE_MAX_CMDS - 1) * sizeof(char *));
 	rs->history[READLINE_MAX_CMDS - 1] = NULL;
 	idx = READLINE_MAX_CMDS - 1;
     }