>From fd3098fe2764b049e23ea125a20979410699d257 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Andr=C3=A9=20Goddard=20Rosa?= <[email protected]>
Date: Sun, 1 Nov 2009 13:46:26 -0200
Subject: [PATCH v2 5/7] vsprintf: reduce code size by avoiding extra check
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
text data bss dec hex filename
15735 0 8 15743 3d7f lib/vsprintf.o-before
15719 0 8 15727 3d6f lib/vsprintf.o-minus-double-check
Signed-off-by: Andr? Goddard Rosa <[email protected]>
---
lib/vsprintf.c | 3 ++-
1 files changed, 2 insertions(+), 1 deletions(-)
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 14e4197..af79152 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -747,8 +747,9 @@ static char *ip6_compressed_string(char *p, const
char *addr)
p = pack_hex_byte(p, hi);
else
*p++ = hex_asc_lo(hi);
+ p = pack_hex_byte(p, lo);
}
- if (hi || lo > 0x0f)
+ else if (lo > 0x0f)
p = pack_hex_byte(p, lo);
else
*p++ = hex_asc_lo(lo);
--
1.6.5.2.140.g5f809
On Mon, Nov 02, 2009 at 03:26:36PM -0200, Andr? Goddard Rosa wrote:
> From fd3098fe2764b049e23ea125a20979410699d257 Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?Andr=C3=A9=20Goddard=20Rosa?= <[email protected]>
> Date: Sun, 1 Nov 2009 13:46:26 -0200
> Subject: [PATCH v2 5/7] vsprintf: reduce code size by avoiding extra check
> MIME-Version: 1.0
> Content-Type: text/plain; charset=UTF-8
> Content-Transfer-Encoding: 8bit
>
> text data bss dec hex filename
> 15735 0 8 15743 3d7f lib/vsprintf.o-before
> 15719 0 8 15727 3d6f lib/vsprintf.o-minus-double-check
>
> Signed-off-by: Andr? Goddard Rosa <[email protected]>
Please add a changelog in your patches, I mean something that
explain your practical change.
I'm not sure this change is necessary, the only impact is a small
reduce of binary code. Well, why not.
> ---
> lib/vsprintf.c | 3 ++-
> 1 files changed, 2 insertions(+), 1 deletions(-)
>
> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> index 14e4197..af79152 100644
> --- a/lib/vsprintf.c
> +++ b/lib/vsprintf.c
> @@ -747,8 +747,9 @@ static char *ip6_compressed_string(char *p, const
> char *addr)
> p = pack_hex_byte(p, hi);
> else
> *p++ = hex_asc_lo(hi);
> + p = pack_hex_byte(p, lo);
> }
> - if (hi || lo > 0x0f)
> + else if (lo > 0x0f)
> p = pack_hex_byte(p, lo);
> else
> *p++ = hex_asc_lo(lo);
> --
> 1.6.5.2.140.g5f809
On Mon, 2009-11-02 at 19:19 +0100, Frederic Weisbecker wrote:
> On Mon, Nov 02, 2009 at 03:26:36PM -0200, André Goddard Rosa wrote:
> > Subject: [PATCH v2 5/7] vsprintf: reduce code size by avoiding extra check
> > text data bss dec hex filename
> > 15735 0 8 15743 3d7f lib/vsprintf.o-before
> > 15719 0 8 15727 3d6f lib/vsprintf.o-minus-double-check
> Please add a changelog in your patches, I mean something that
> explain your practical change.
Read the subject and try not to go overboard.
I think the changelog André provided is perfect.
On Mon, Nov 02, 2009 at 10:29:43AM -0800, Joe Perches wrote:
> On Mon, 2009-11-02 at 19:19 +0100, Frederic Weisbecker wrote:
> > On Mon, Nov 02, 2009 at 03:26:36PM -0200, Andr? Goddard Rosa wrote:
> > > Subject: [PATCH v2 5/7] vsprintf: reduce code size by avoiding extra check
> > > text data bss dec hex filename
> > > 15735 0 8 15743 3d7f lib/vsprintf.o-before
> > > 15719 0 8 15727 3d6f lib/vsprintf.o-minus-double-check
> > Please add a changelog in your patches, I mean something that
> > explain your practical change.
>
> Read the subject and try not to go overboard.
> I think the changelog Andr? provided is perfect.
It took me some time yesterday to understand how ip6_compressed_string()
does its job, hence it took me some time to ensure this patch is safe,
hence this reaction for something that didn't seem obvious to me at
a first glance.
But you're right actually, the problem was more in my homework than in
the changelog.
* Frederic Weisbecker <[email protected]> wrote:
> On Mon, Nov 02, 2009 at 10:29:43AM -0800, Joe Perches wrote:
> > On Mon, 2009-11-02 at 19:19 +0100, Frederic Weisbecker wrote:
> > > On Mon, Nov 02, 2009 at 03:26:36PM -0200, Andr? Goddard Rosa wrote:
> > > > Subject: [PATCH v2 5/7] vsprintf: reduce code size by avoiding extra check
> > > > text data bss dec hex filename
> > > > 15735 0 8 15743 3d7f lib/vsprintf.o-before
> > > > 15719 0 8 15727 3d6f lib/vsprintf.o-minus-double-check
> > > Please add a changelog in your patches, I mean something that
> > > explain your practical change.
> >
> > Read the subject and try not to go overboard.
> > I think the changelog Andr? provided is perfect.
>
>
> It took me some time yesterday to understand how
> ip6_compressed_string() does its job, hence it took me some time to
> ensure this patch is safe, hence this reaction for something that
> didn't seem obvious to me at a first glance.
>
> But you're right actually, the problem was more in my homework than in
> the changelog.
Since you wrote the last iteration of that bit and you didnt find it
trivial i'd strongly suggest the next version of this patch to include a
more verbose changelog that explains what is being done. There's no harm
in being verbose about reasons for a change - and there's a lot of harm
from being too short.
Ingo
* Frederic Weisbecker <[email protected]> wrote:
> I'm not sure this change is necessary, the only impact is a small
> reduce of binary code. Well, why not.
Small reductions in size are nice too. They counter-act small increases
in kernel size. (which are way more common unfortunately)
Ingo
On Mon, Nov 02, 2009 at 08:36:23PM +0100, Ingo Molnar wrote:
>
> * Frederic Weisbecker <[email protected]> wrote:
>
> > On Mon, Nov 02, 2009 at 10:29:43AM -0800, Joe Perches wrote:
> > > On Mon, 2009-11-02 at 19:19 +0100, Frederic Weisbecker wrote:
> > > > On Mon, Nov 02, 2009 at 03:26:36PM -0200, Andr? Goddard Rosa wrote:
> > > > > Subject: [PATCH v2 5/7] vsprintf: reduce code size by avoiding extra check
> > > > > text data bss dec hex filename
> > > > > 15735 0 8 15743 3d7f lib/vsprintf.o-before
> > > > > 15719 0 8 15727 3d6f lib/vsprintf.o-minus-double-check
> > > > Please add a changelog in your patches, I mean something that
> > > > explain your practical change.
> > >
> > > Read the subject and try not to go overboard.
> > > I think the changelog Andr? provided is perfect.
> >
> >
> > It took me some time yesterday to understand how
> > ip6_compressed_string() does its job, hence it took me some time to
> > ensure this patch is safe, hence this reaction for something that
> > didn't seem obvious to me at a first glance.
> >
> > But you're right actually, the problem was more in my homework than in
> > the changelog.
>
> Since you wrote the last iteration of that bit and you didnt find it
Andr? wrote this patch, not me :)
> trivial i'd strongly suggest the next version of this patch to include a
> more verbose changelog that explains what is being done. There's no harm
> in being verbose about reasons for a change - and there's a lot of harm
> from being too short.
>
> Ingo
* Frederic Weisbecker <[email protected]> wrote:
> On Mon, Nov 02, 2009 at 08:36:23PM +0100, Ingo Molnar wrote:
> >
> > * Frederic Weisbecker <[email protected]> wrote:
> >
> > > On Mon, Nov 02, 2009 at 10:29:43AM -0800, Joe Perches wrote:
> > > > On Mon, 2009-11-02 at 19:19 +0100, Frederic Weisbecker wrote:
> > > > > On Mon, Nov 02, 2009 at 03:26:36PM -0200, Andr? Goddard Rosa wrote:
> > > > > > Subject: [PATCH v2 5/7] vsprintf: reduce code size by avoiding extra check
> > > > > > text data bss dec hex filename
> > > > > > 15735 0 8 15743 3d7f lib/vsprintf.o-before
> > > > > > 15719 0 8 15727 3d6f lib/vsprintf.o-minus-double-check
> > > > > Please add a changelog in your patches, I mean something that
> > > > > explain your practical change.
> > > >
> > > > Read the subject and try not to go overboard.
> > > > I think the changelog Andr? provided is perfect.
> > >
> > >
> > > It took me some time yesterday to understand how
> > > ip6_compressed_string() does its job, hence it took me some time to
> > > ensure this patch is safe, hence this reaction for something that
> > > didn't seem obvious to me at a first glance.
> > >
> > > But you're right actually, the problem was more in my homework than in
> > > the changelog.
> >
> > Since you wrote the last iteration of that bit and you didnt find it
>
>
> Andr? wrote this patch, not me :)
i know - i mean you touched the vsprintf code last materially.
But i see that there's been this commit since then:
8a27f7c: lib/vsprintf.c: Add "%pI6c" - print pointer as compressed ipv6 address
which introduced ip6_compressed_string().
Ingo