Обсуждение: cleanup of cbrt() handling

This patch cleans up our static cbrt() implementation in float.c.

--
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 359-1001
  +  If your life is a hard drive,     |  13 Roberts Road
  +  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073
Index: src/backend/utils/adt/float.c
===================================================================
RCS file: /cvsroot/pgsql-server/src/backend/utils/adt/float.c,v
retrieving revision 1.87
diff -c -c -r1.87 float.c
*** src/backend/utils/adt/float.c    9 May 2003 21:19:49 -0000    1.87
--- src/backend/utils/adt/float.c    25 May 2003 05:28:58 -0000
***************
*** 70,93 ****
  #include "utils/builtins.h"


- #if !(NeXT && NX_CURRENT_COMPILER_RELEASE > NX_COMPILER_RELEASE_3_2)
-  /* NS3.3 has conflicting declarations of these in <math.h> */
-
- #ifndef atof
- extern double atof(const char *p);
- #endif
-
  #ifndef HAVE_CBRT
- #define cbrt my_cbrt
  static double cbrt(double x);
-
- #else
- #if !defined(nextstep)
- extern double cbrt(double x);
- #endif
  #endif   /* HAVE_CBRT */
- #endif   /* NeXT check */
-

  #ifndef M_PI
  /* from my RH5.2 gcc math.h file - thomas 2000-04-03 */
--- 70,78 ----
***************
*** 1983,1989 ****
  /* ========== PRIVATE ROUTINES ========== */

  #ifndef HAVE_CBRT
!
  static double
  cbrt(double x)
  {
--- 1968,1974 ----
  /* ========== PRIVATE ROUTINES ========== */

  #ifndef HAVE_CBRT
! /* I doubt this is still needed by any platform.  2003-05-25 */
  static double
  cbrt(double x)
  {

Bruce Momjian <pgman@candle.pha.pa.us> writes:
> This patch cleans up our static cbrt() implementation in float.c.

Looks like an improvement to me.

            regards, tom lane

Tom Lane wrote:
> Bruce Momjian <pgman@candle.pha.pa.us> writes:
> > This patch cleans up our static cbrt() implementation in float.c.
>
> Looks like an improvement to me.

Yep, however, now that I look at the original code, and code that
shipped in 7.3:

    #ifndef HAVE_CBRT
    #define cbrt my_cbrt
    static double cbrt(double x);

    #else
    #if !defined(nextstep)
    extern double cbrt(double x);
    #endif
    #endif   /* HAVE_CBRT */

There is no my_cbrt() function, meaning anyone who didn't have cbrt
couldn't have even compiled 7.3, so I think we should just remove cbrt,
or mark it as UNUSED.

--
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 359-1001
  +  If your life is a hard drive,     |  13 Roberts Road
  +  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073

Bruce Momjian <pgman@candle.pha.pa.us> writes:
>     #ifndef HAVE_CBRT
>     #define cbrt my_cbrt
>     static double cbrt(double x);

>     #else
>     #if !defined(nextstep)
>     extern double cbrt(double x);
>     #endif
>     #endif   /* HAVE_CBRT */

> There is no my_cbrt() function, meaning anyone who didn't have cbrt
> couldn't have even compiled 7.3, so I think we should just remove
> cbrt,

No, you're misreading the point of the code.  The #define changes the
spelling of the static declaration.  The idea evidently is to make sure
that there is no conflict of the static function against a library
cbrt(), on the off chance that configure missed finding it somehow.
This might be overly tricky --- certainly we do not take comparable
precautions for other library-substitute functions.  I wouldn't object
to removing the "#define cbrt my_cbrt".  But you have *no* proof that
removing the whole thing won't break some supported platform.

            regards, tom lane

Tom Lane wrote:
> Bruce Momjian <pgman@candle.pha.pa.us> writes:
> >     #ifndef HAVE_CBRT
> >     #define cbrt my_cbrt
> >     static double cbrt(double x);
>
> >     #else
> >     #if !defined(nextstep)
> >     extern double cbrt(double x);
> >     #endif
> >     #endif   /* HAVE_CBRT */
>
> > There is no my_cbrt() function, meaning anyone who didn't have cbrt
> > couldn't have even compiled 7.3, so I think we should just remove
> > cbrt,
>
> No, you're misreading the point of the code.  The #define changes the
> spelling of the static declaration.  The idea evidently is to make sure
> that there is no conflict of the static function against a library
> cbrt(), on the off chance that configure missed finding it somehow.
> This might be overly tricky --- certainly we do not take comparable
> precautions for other library-substitute functions.  I wouldn't object
> to removing the "#define cbrt my_cbrt".  But you have *no* proof that
> removing the whole thing won't break some supported platform.

Oh, I see, it makes all cbrt cases by my_cbrt, including the function
calls _and_ function definition.  I already removed my_cbrt, so let's
see how it works in 7.4.

--
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 359-1001
  +  If your life is a hard drive,     |  13 Roberts Road
  +  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073