‘pi’ shadows a global declaration

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
6 messages Options
Reply | Threaded
Open this post in threaded view
|

‘pi’ shadows a global declaration

Dmitri A. Sergatskov
Probably no a big problem. Mostly fyi:

g++ -DHAVE_CONFIG_H -I. -I../src -Iliboctave -I../src/liboctave -I../src/liboctave/array -Iliboctave/numeric -I../src/liboctave/numeric -Iliboctave/operators -I../src/liboctave/operators -I../src/liboctave/system -I../src/liboctave/util -I../src/liboctave/wrappers -I../src/liboctave/external/Faddeeva -fPIC -pthread -fopenmp -Wall -W -Wshadow -Wold-style-cast -Wformat -Wpointer-arith -Wwrite-strings -Wcast-align -Wcast-qual -g -O2 -MT liboctave/numeric/liboctave_numeric_libnumeric_la-oct-convn.lo -MD -MP -MF liboctave/numeric/.deps/liboctave_numeric_libnumeric_la-oct-convn.Tpo -c ../src/liboctave/numeric/oct-convn.cc  -fPIC -DPIC -o liboctave/numeric/.libs/liboctave_numeric_libnumeric_la-oct-convn.o
../src/liboctave/numeric/lo-specfun.cc: In function ‘Complex octave::math::rc_log1p(double)’:
../src/liboctave/numeric/lo-specfun.cc:3452:20: warning: declaration of ‘pi’ shadows a global declaration [-Wshadow]
       const double pi = 3.14159265358979323846;
                    ^~
../src/liboctave/numeric/lo-specfun.cc:3198:25: note: shadowed declaration is here
     static const double pi = 3.14159265358979323846;
                         ^~
../src/liboctave/numeric/lo-specfun.cc: In function ‘FloatComplex octave::math::rc_log1p(float)’:
../src/liboctave/numeric/lo-specfun.cc:3460:19: warning: declaration of ‘pi’ shadows a global declaration [-Wshadow]
       const float pi = 3.14159265358979323846f;
                   ^~
../src/liboctave/numeric/lo-specfun.cc:3198:25: note: shadowed declaration is here
     static const double pi = 3.14159265358979323846;
                         ^~

Dmitri.
--

Reply | Threaded
Open this post in threaded view
|

Re: ‘pi’ shadows a global declaration

John W. Eaton
Administrator
On 07/09/2017 11:30 PM, Dmitri A. Sergatskov wrote:
> Probably no a big problem. Mostly fyi:

> ../src/liboctave/numeric/lo-specfun.cc:3460:19: warning: declaration of
> ‘pi’ shadows a global declaration [-Wshadow] const float pi =
> 3.14159265358979323846f; ^~
> ../src/liboctave/numeric/lo-specfun.cc:3198:25: note: shadowed
> declaration is here static const double pi = 3.14159265358979323846; ^~

Yeah, I noticed that.  We currently use a mixture of these kinds of
locally defined constants and macros like M_PI that may be defined by
math.h.  Since we generally want to avoid macros from C-language headers
in C++ code we try to only use C++ headers instead of C headers (<cmath>
instead of <math.h>) but we also define _GNU_SOURCE, which currently
causes macros like M_PI to be defined, at least on some systems.

So we could just use the macros, but I'm not sure that we should depend
on them as they are not standard for C++ code.  Maybe we should define
our own header file for math constants and use them consistently?

Whatever we do, I'd prefer to use the same constants in all Octave code.

jwe


Reply | Threaded
Open this post in threaded view
|

Re: ‘pi’ shadows a global declaration

Dmitri A. Sergatskov
On Mon, Jul 10, 2017 at 8:19 AM, John W. Eaton <[hidden email]> wrote:


../src/liboctave/numeric/lo-specfun.cc:3460:19: warning: declaration of
‘pi’ shadows a global declaration [-Wshadow] const float pi =
3.14159265358979323846f; ^~
../src/liboctave/numeric/lo-specfun.cc:3198:25: note: shadowed
declaration is here static const double pi = 3.14159265358979323846; ^~

Yeah, I noticed that.  We currently use a mixture of these kinds of locally defined constants and macros like M_PI that may be defined by math.h.  Since we generally want to avoid macros from C-language headers in C++ code we try to only use C++ headers instead of C headers (<cmath> instead of <math.h>) but we also define _GNU_SOURCE, which currently causes macros like M_PI to be defined, at least on some systems.

So we could just use the macros, but I'm not sure that we should depend on them as they are not standard for C++ code.  Maybe we should define our own header file for math constants and use them consistently?

Whatever we do, I'd prefer to use the same constants in all Octave code.

jwe


​I have seen (e.g. [1]) people sayihttps://gcc.gnu.org/onlinedocs/gcc-6.1.0/libstdc++/api/a01120_source.htmlng that one should use constexpr to define math constants in C++11 and beyond.

Not sure if that helps in our particular case.

[1] ​https://gcc.gnu.org/onlinedocs/gcc-6.1.0/libstdc++/api/a01120_source.html

Dmitri.
--


Reply | Threaded
Open this post in threaded view
|

Re: ‘pi’ shadows a global declaration

Rik-4
In reply to this post by Dmitri A. Sergatskov
On 07/10/2017 09:00 AM, [hidden email] wrote:
Subject:
Re: ‘pi’ shadows a global declaration
From:
"John W. Eaton" [hidden email]
Date:
07/10/2017 06:19 AM
To:
"Dmitri A. Sergatskov" [hidden email], octave-maintainers [hidden email]
List-Post:
[hidden email]
Content-Transfer-Encoding:
quoted-printable
Precedence:
list
MIME-Version:
1.0
References:
[hidden email]
In-Reply-To:
[hidden email]
Message-ID:
[hidden email]
Content-Type:
text/plain; charset=utf-8; format=flowed
Message:
2

On 07/09/2017 11:30 PM, Dmitri A. Sergatskov wrote:
Probably no a big problem. Mostly fyi:

../src/liboctave/numeric/lo-specfun.cc:3460:19: warning: declaration of
‘pi’ shadows a global declaration [-Wshadow] const float pi =
3.14159265358979323846f; ^~
../src/liboctave/numeric/lo-specfun.cc:3198:25: note: shadowed
declaration is here static const double pi = 3.14159265358979323846; ^~

Yeah, I noticed that.  We currently use a mixture of these kinds of locally defined constants and macros like M_PI that may be defined by math.h.  Since we generally want to avoid macros from C-language headers in C++ code we try to only use C++ headers instead of C headers (<cmath> instead of <math.h>) but we also define _GNU_SOURCE, which currently causes macros like M_PI to be defined, at least on some systems.

We guarantee in configure.ac that these constants are available.  See the following check:

if test $octave_cv_header_math_defines = yes; then
  AC_DEFINE(HAVE_MATH_DEFINES, 1,
    [Define to 1 if defines such as M_PI are available in math.h])
else
  AC_MSG_ERROR([MATH DEFINES in math.h such as M_PI are required to build Octave])
fi


So we could just use the macros, but I'm not sure that we should depend on them as they are not standard for C++ code.  Maybe we should define our own header file for math constants and use them consistently?

Almost everywhere we care to build Octave these constants are a part of <cmath>.  We could do this, but we really haven't had any problem to date.  I would let sleeping dogs lie.


Whatever we do, I'd prefer to use the same constants in all Octave code.

I agree.  At the moment, I would prefer to get rid of these constants in favor of the generic ones in math.h/cmath.  I already started doing that over on the liboctave side about two weeks ago in this cset:

changeset:   23682:6fe13cd3543c
user:        Rik [hidden email]
date:        Fri Jun 23 12:32:19 2017 -0700
files:       liboctave/numeric/lo-mappers.cc liboctave/numeric/lo-mappers.h liboctave/numeric/lo-specfun.cc
description:
Clean up lo-mappers.h, lo-mappers.cc.

Here is a representative change from that cset:

     FloatComplex
     log2 (const FloatComplex& x)
     {
-#if defined (M_LN2)
-      static float ln2 = M_LN2;
-#else
-      static float ln2 = log (2.0f);
-#endif
-      return std::log (x) / ln2;
+      return std::log (x) / static_cast<float> (M_LN2);
     }

--Rik

Reply | Threaded
Open this post in threaded view
|

Re: ‘pi’ shadows a global declaration

John W. Eaton
Administrator
On 07/10/2017 01:24 PM, Rik wrote:

> We guarantee in configure.ac that these constants are available.  See
> the following check:
>
> if test $octave_cv_header_math_defines = yes; then
>   AC_DEFINE(HAVE_MATH_DEFINES, 1,
>     [Define to 1 if defines such as M_PI are available in math.h])
> else
>   AC_MSG_ERROR([MATH DEFINES in math.h such as M_PI are required to
> build Octave])
> fi
>
>>
>> So we could just use the macros, but I'm not sure that we should
>> depend on them as they are not standard for C++ code.  Maybe we should
>> define our own header file for math constants and use them consistently?
>
> Almost everywhere we care to build Octave these constants are a part of
> <cmath>.  We could do this, but we really haven't had any problem to
> date.  I would let sleeping dogs lie.
>
>>
>> Whatever we do, I'd prefer to use the same constants in all Octave code.
>
> I agree.  At the moment, I would prefer to get rid of these constants in
> favor of the generic ones in math.h/cmath.  I already started doing that
> over on the liboctave side about two weeks ago in this cset:
>
> changeset:   23682:6fe13cd3543c
> user:        Rik <[hidden email]>
> date:        Fri Jun 23 12:32:19 2017 -0700
> files:       liboctave/numeric/lo-mappers.cc
> liboctave/numeric/lo-mappers.h liboctave/numeric/lo-specfun.cc
> description:
> Clean up lo-mappers.h, lo-mappers.cc.
>
> Here is a representative change from that cset:
>
>      FloatComplex
>      log2 (const FloatComplex& x)
>      {
> -#if defined (M_LN2)
> -      static float ln2 = M_LN2;
> -#else
> -      static float ln2 = log (2.0f);
> -#endif
> -      return std::log (x) / ln2;
> +      return std::log (x) / static_cast<float> (M_LN2);
>      }

Thanks for looking at this.  Instead of failing in configure and
duplicating the #ifdef logic everywhere they are needed, why not just
ensure that the necessary constants are available somewhere?

jwe


Reply | Threaded
Open this post in threaded view
|

Re: ‘pi’ shadows a global declaration

Rik-4
On 07/10/2017 11:01 AM, John W. Eaton wrote:

> On 07/10/2017 01:24 PM, Rik wrote:
>
>> We guarantee in configure.ac that these constants are available.  See
>> the following check:
>>
>> if test $octave_cv_header_math_defines = yes; then
>>   AC_DEFINE(HAVE_MATH_DEFINES, 1,
>>     [Define to 1 if defines such as M_PI are available in math.h])
>> else
>>   AC_MSG_ERROR([MATH DEFINES in math.h such as M_PI are required to
>> build Octave])
>> fi
>>
>>>
>>> So we could just use the macros, but I'm not sure that we should
>>> depend on them as they are not standard for C++ code.  Maybe we should
>>> define our own header file for math constants and use them consistently?
>>
>> Almost everywhere we care to build Octave these constants are a part of
>> <cmath>.  We could do this, but we really haven't had any problem to
>> date.  I would let sleeping dogs lie.
>>
>>>
>>> Whatever we do, I'd prefer to use the same constants in all Octave code.
>>
>> I agree.  At the moment, I would prefer to get rid of these constants in
>> favor of the generic ones in math.h/cmath.  I already started doing that
>> over on the liboctave side about two weeks ago in this cset:
>>
>> changeset:   23682:6fe13cd3543c
>> user:        Rik <[hidden email]>
>> date:        Fri Jun 23 12:32:19 2017 -0700
>> files:       liboctave/numeric/lo-mappers.cc
>> liboctave/numeric/lo-mappers.h liboctave/numeric/lo-specfun.cc
>> description:
>> Clean up lo-mappers.h, lo-mappers.cc.
>>
>> Here is a representative change from that cset:
>>
>>      FloatComplex
>>      log2 (const FloatComplex& x)
>>      {
>> -#if defined (M_LN2)
>> -      static float ln2 = M_LN2;
>> -#else
>> -      static float ln2 = log (2.0f);
>> -#endif
>> -      return std::log (x) / ln2;
>> +      return std::log (x) / static_cast<float> (M_LN2);
>>      }
>
> Thanks for looking at this.  Instead of failing in configure and
> duplicating the #ifdef logic everywhere they are needed, why not just
> ensure that the necessary constants are available somewhere?

My changeset eliminated the #ifdef logic.  That was the simplification in
the  C++ files.  The configure test ensures that the necessary constants
are available, and we've never had a reported problem with the systems we
build for.  It's not quite lazy, but there doesn't seem to be a need to
overengineer this.  If someone does find a platform where this fails then
it will be early on in configure and we can decide what we want to do.
There are options available at that time.  We could get math.h from gnulib
or define the constants ourselves.

--Rik