stdcxx-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Martin Sebor <se...@roguewave.com>
Subject Re: Please peer review this change to src/num_put.cpp
Date Wed, 21 May 2008 21:45:17 GMT
Eric Lemings wrote:
> I'm fixing 64-bit conversion warnings per STDCXX-550.  One change I've
> made is to src/num_put.cpp.  Thought it would be a good idea to get
> another set of eyes on it since it's a change to library source:
>  
> Index: src/num_put.cpp
> ===================================================================
> --- src/num_put.cpp     (revision 657873)
> +++ src/num_put.cpp     (working copy)
> @@ -122,7 +122,7 @@
>  inline bool __rw_signbit (double val)
>  {
>      // implement own signbit() to avoid dragging in libm or libsunmath
> -    return _RWSTD_REINTERPRET_CAST (const _RWSTD_UINT64_T&, val) >> 63;
> +    return bool (_RWSTD_REINTERPRET_CAST (_RWSTD_UINT64_T&, val) >>
> 63);

Rather than a cast, I would suggest something like:

     // for convenience
     typedef _RWSTD_UINT64_T UInt64T;

     const UInt64T ival = _RWSTD_REINTERPRET_CAST (UInt64T&, val);

     return 0 != (ival & (UInt64T (1) << 63));

The shift will get computed at compile time and the bitwise
AND might be a few CPU cycles faster. Before committing this
you might want to check the optimized assembly to make sure
the change doesn't introduce a performance regression.

>  }
>  
>  inline bool __rw_isinf (double val) {
> @@ -622,7 +622,7 @@
>          j = 0;
>  
>      do {
> -        const unsigned dig = (i >> (j * bits)) & basemask;

I don't think the compiler warning is justified here. AFAICS,
the type of basemask is unsigned int, so regardless of the
type of the LHS operand (unsigned long here), the result is
guaranteed to fit into an unsigned int. I suspect the warning
doesn't take into account the semantics of the expressions,
just the types. The cast seems like the only way to shut it
up.

Martin

> +        const unsigned dig = unsigned ((i >> (j * bits)) & basemask);
>  
>          _RWSTD_ASSERT (dig <= basemask);
>  
> I think the explicit bool cast should be fine since most compilers will
> do it anyway.  Same with the explicit unsigned cast.  Or should I use
> _RWSTD_STATIC_CAST?
>  
> Brad.


Mime
View raw message