Patchwork [6,of,8,faster-obsmarkers] getbefloat64: drop unnecessary memcpy

login
register
mail settings
Submitter Augie Fackler
Date Feb. 2, 2015, 4:01 p.m.
Message ID <22d73e82c50d1126c764.1422892887@arthedain.pit.corp.google.com>
Download mbox | patch
Permalink /patch/7600/
State Superseded
Headers show

Comments

Augie Fackler - Feb. 2, 2015, 4:01 p.m.
# HG changeset patch
# User Martin von Zweigbergk <martinvonz@google.com>
# Date 1421790379 28800
#      Tue Jan 20 13:46:19 2015 -0800
# Branch stable
# Node ID 22d73e82c50d1126c7640d960cf74fcf328add6c
# Parent  bbd5e20889d06d6dccc8b3e83946d65d0cc79570
getbefloat64: drop unnecessary memcpy
Augie Fackler - Feb. 2, 2015, 4:11 p.m.
I originally had written this, but then someone mumbled something
about strict aliasing rules in C. Since I have no idea if this is
actually legal, I left it as its own patch.

On Mon, Feb 2, 2015 at 11:01 AM, Augie Fackler <raf@durin42.com> wrote:
> # HG changeset patch
> # User Martin von Zweigbergk <martinvonz@google.com>
> # Date 1421790379 28800
> #      Tue Jan 20 13:46:19 2015 -0800
> # Branch stable
> # Node ID 22d73e82c50d1126c7640d960cf74fcf328add6c
> # Parent  bbd5e20889d06d6dccc8b3e83946d65d0cc79570
> getbefloat64: drop unnecessary memcpy
>
> diff --git a/mercurial/util.h b/mercurial/util.h
> --- a/mercurial/util.h
> +++ b/mercurial/util.h
> @@ -199,14 +199,12 @@ static inline void putbe32(uint32_t x, c
>  static inline double getbefloat64(const char *c)
>  {
>         const unsigned char *d = (const unsigned char *)c;
> -       double ret;
>         int i;
>         uint64_t t = 0;
>         for (i = 0; i < 8; i++) {
>                 t = (t<<8) + d[i];
>         }
> -       memcpy(&ret, &t, sizeof(t));
> -       return ret;
> +       return *((const double*)&t);
>  }
>
>  #endif /* _HG_UTIL_H_ */
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@selenic.com
> http://selenic.com/mailman/listinfo/mercurial-devel
Yuya Nishihara - Feb. 3, 2015, 2 p.m.
On Mon, 2 Feb 2015 11:11:37 -0500, Augie Fackler wrote:
> I originally had written this, but then someone mumbled something
> about strict aliasing rules in C. Since I have no idea if this is
> actually legal, I left it as its own patch.
> 
> On Mon, Feb 2, 2015 at 11:01 AM, Augie Fackler <raf@durin42.com> wrote:
> > # HG changeset patch
> > # User Martin von Zweigbergk <martinvonz@google.com>
> > # Date 1421790379 28800
> > #      Tue Jan 20 13:46:19 2015 -0800
> > # Branch stable
> > # Node ID 22d73e82c50d1126c7640d960cf74fcf328add6c
> > # Parent  bbd5e20889d06d6dccc8b3e83946d65d0cc79570
> > getbefloat64: drop unnecessary memcpy
> >
> > diff --git a/mercurial/util.h b/mercurial/util.h
> > --- a/mercurial/util.h
> > +++ b/mercurial/util.h
> > @@ -199,14 +199,12 @@ static inline void putbe32(uint32_t x, c
> >  static inline double getbefloat64(const char *c)
> >  {
> >         const unsigned char *d = (const unsigned char *)c;
> > -       double ret;
> >         int i;
> >         uint64_t t = 0;
> >         for (i = 0; i < 8; i++) {
> >                 t = (t<<8) + d[i];
> >         }
> > -       memcpy(&ret, &t, sizeof(t));
> > -       return ret;
> > +       return *((const double*)&t);

IIRC, common idiom is to use union { double, uint64_t }.

But Python itself violates strict aliasing rules, so maybe we can expect
that -fno-strict-aliasing or something is always set.

https://hg.python.org/cpython/file/v2.7.9/configure#l5983

Regards,
Julien Cristau - Feb. 3, 2015, 2:12 p.m.
On Tue, Feb  3, 2015 at 23:00:38 +0900, Yuya Nishihara wrote:

> On Mon, 2 Feb 2015 11:11:37 -0500, Augie Fackler wrote:
> > I originally had written this, but then someone mumbled something
> > about strict aliasing rules in C. Since I have no idea if this is
> > actually legal, I left it as its own patch.
> > 
> > On Mon, Feb 2, 2015 at 11:01 AM, Augie Fackler <raf@durin42.com> wrote:
> > > # HG changeset patch
> > > # User Martin von Zweigbergk <martinvonz@google.com>
> > > # Date 1421790379 28800
> > > #      Tue Jan 20 13:46:19 2015 -0800
> > > # Branch stable
> > > # Node ID 22d73e82c50d1126c7640d960cf74fcf328add6c
> > > # Parent  bbd5e20889d06d6dccc8b3e83946d65d0cc79570
> > > getbefloat64: drop unnecessary memcpy
> > >
> > > diff --git a/mercurial/util.h b/mercurial/util.h
> > > --- a/mercurial/util.h
> > > +++ b/mercurial/util.h
> > > @@ -199,14 +199,12 @@ static inline void putbe32(uint32_t x, c
> > >  static inline double getbefloat64(const char *c)
> > >  {
> > >         const unsigned char *d = (const unsigned char *)c;
> > > -       double ret;
> > >         int i;
> > >         uint64_t t = 0;
> > >         for (i = 0; i < 8; i++) {
> > >                 t = (t<<8) + d[i];
> > >         }
> > > -       memcpy(&ret, &t, sizeof(t));
> > > -       return ret;
> > > +       return *((const double*)&t);
> 
> IIRC, common idiom is to use union { double, uint64_t }.
> 
> But Python itself violates strict aliasing rules, so maybe we can expect
> that -fno-strict-aliasing or something is always set.
> 
> https://hg.python.org/cpython/file/v2.7.9/configure#l5983
> 
-fno-strict-aliasing can't guarantee correct alignment, though, so
unless you're sure the const char * is always 8-byte aligned you can't
just cast it.  IOW, the union is probably safest.

Julien
Augie Fackler - Feb. 3, 2015, 3:56 p.m.
On Tue, Feb 3, 2015 at 9:12 AM, Julien Cristau
<julien.cristau@logilab.fr> wrote:
> -fno-strict-aliasing can't guarantee correct alignment, though, so
> unless you're sure the const char * is always 8-byte aligned you can't
> just cast it.  IOW, the union is probably safest.


I'm confused how the char* relates to the caast of the uint64_t to double?

I'll do a resend with a union.
Julien Cristau - Feb. 3, 2015, 4:22 p.m.
On Tue, Feb  3, 2015 at 10:56:05 -0500, Augie Fackler wrote:

> On Tue, Feb 3, 2015 at 9:12 AM, Julien Cristau
> <julien.cristau@logilab.fr> wrote:
> > -fno-strict-aliasing can't guarantee correct alignment, though, so
> > unless you're sure the const char * is always 8-byte aligned you can't
> > just cast it.  IOW, the union is probably safest.
> 
> 
> I'm confused how the char* relates to the caast of the uint64_t to double?
> 
I think my brain somehow confused t and d.

Cheers,
Julien

Patch

diff --git a/mercurial/util.h b/mercurial/util.h
--- a/mercurial/util.h
+++ b/mercurial/util.h
@@ -199,14 +199,12 @@  static inline void putbe32(uint32_t x, c
 static inline double getbefloat64(const char *c)
 {
 	const unsigned char *d = (const unsigned char *)c;
-	double ret;
 	int i;
 	uint64_t t = 0;
 	for (i = 0; i < 8; i++) {
 		t = (t<<8) + d[i];
 	}
-	memcpy(&ret, &t, sizeof(t));
-	return ret;
+	return *((const double*)&t);
 }
 
 #endif /* _HG_UTIL_H_ */