Am Fr., 16. Nov. 2018 um 18:13 Uhr schrieb Ed Smith-Rowland
<[hidden email]>: > > Greetings, > > This is late but I wanted to put it out there just to finish a thing. > > It's fairly straightforward constexpr of operators and some simple > functions for std::complex. > > The only thing that jumped out was the norm function. We had this: > > struct _Norm_helper<true> > { > template<typename _Tp> > static inline _Tp _S_do_it(const complex<_Tp>& __z) > { > _Tp __res = std::abs(__z); > return __res * __res; > } > }; > > Since abs can't be made constexpr for complex since it involves sqrt (It > probably could but that's another story) I had to fall back to the x^2 + > y^2. I don't know who that will bother. This version should be faster > and I can't think of any useful trustworthy difference numerically > either in terms of accuracy of stability. > > Barring any feedback on that I'll clean it up and maybe rename my tests > from constexpr_all_the_things.cc to more_constexpr.cc ;-) > > It builds and tests cleanly on x86_64-linux. Hmmh, according to the recent working draft the following complex functions are *not* constexpr: arg, proj So, shouldn't their new C++20-constexpr specifiers be added conditionally (In the sense of gcc extensions)? - Daniel |
On 11/16/18 12:38 PM, Daniel Krügler wrote:
> Am Fr., 16. Nov. 2018 um 18:13 Uhr schrieb Ed Smith-Rowland > <[hidden email]>: >> Greetings, >> >> This is late but I wanted to put it out there just to finish a thing. >> >> It's fairly straightforward constexpr of operators and some simple >> functions for std::complex. >> >> The only thing that jumped out was the norm function. We had this: >> >> struct _Norm_helper<true> >> { >> template<typename _Tp> >> static inline _Tp _S_do_it(const complex<_Tp>& __z) >> { >> _Tp __res = std::abs(__z); >> return __res * __res; >> } >> }; >> >> Since abs can't be made constexpr for complex since it involves sqrt (It >> probably could but that's another story) I had to fall back to the x^2 + >> y^2. I don't know who that will bother. This version should be faster >> and I can't think of any useful trustworthy difference numerically >> either in terms of accuracy of stability. >> >> Barring any feedback on that I'll clean it up and maybe rename my tests >> from constexpr_all_the_things.cc to more_constexpr.cc ;-) >> >> It builds and tests cleanly on x86_64-linux. > Hmmh, according to the recent working draft the following complex > functions are *not* constexpr: > > arg, proj > > So, shouldn't their new C++20-constexpr specifiers be added > conditionally (In the sense of gcc extensions)? > > - Daniel > and proj I'll remove the constexpr on those and adjust everything. Thank you. Ed |
On 11/16/18 3:53 PM, Ed Smith-Rowland wrote:
> On 11/16/18 12:38 PM, Daniel Krügler wrote: >> Am Fr., 16. Nov. 2018 um 18:13 Uhr schrieb Ed Smith-Rowland >> <[hidden email]>: >>> Greetings, >>> >>> This is late but I wanted to put it out there just to finish a thing. >>> >>> It's fairly straightforward constexpr of operators and some simple >>> functions for std::complex. >>> >>> The only thing that jumped out was the norm function. We had this: >>> >>> struct _Norm_helper<true> >>> { >>> template<typename _Tp> >>> static inline _Tp _S_do_it(const complex<_Tp>& __z) >>> { >>> _Tp __res = std::abs(__z); >>> return __res * __res; >>> } >>> }; >>> >>> Since abs can't be made constexpr for complex since it involves sqrt >>> (It >>> probably could but that's another story) I had to fall back to the >>> x^2 + >>> y^2. I don't know who that will bother. This version should be faster >>> and I can't think of any useful trustworthy difference numerically >>> either in terms of accuracy of stability. >>> >>> Barring any feedback on that I'll clean it up and maybe rename my tests >>> from constexpr_all_the_things.cc to more_constexpr.cc ;-) >>> >>> It builds and tests cleanly on x86_64-linux. >> Hmmh, according to the recent working draft the following complex >> functions are *not* constexpr: >> >> arg, proj >> >> So, shouldn't their new C++20-constexpr specifiers be added >> conditionally (In the sense of gcc extensions)? >> >> - Daniel >> > I did not see that those were not constexpr. I guess arg needs atan2 > and proj > > I'll remove the constexpr on those and adjust everything. > > Thank you. > > Ed > Here is the cleaned up, conformant patch. Builds and tests cleanly on x86_64-linux. Is this OK or should I really wait? Ed |
On 16/11/18 19:39 -0500, Ed Smith-Rowland wrote:
>@@ -322,67 +323,43 @@ > //@{ > /// Return new complex value @a x plus @a y. > template<typename _Tp> >- inline complex<_Tp> >+ inline _GLIBCXX20_CONSTEXPR complex<_Tp> > operator+(const complex<_Tp>& __x, const complex<_Tp>& __y) >- { >- complex<_Tp> __r = __x; >- __r += __y; >- return __r; >- } >+ { return complex<_Tp>(__x.real() + __y.real(), __x.imag() + __y.imag()); } Is this change (and all the similar ones) really needed? Doesn't the fact that all the constructors and member operators of std::complex mean that the original definition is also valid in a constexpr function? >@@ -1163,50 +1143,43 @@ > #endif > > template<typename _Tp> >- complex& >+ _GLIBCXX20_CONSTEXPR complex& > operator=(const complex<_Tp>& __z) > { >- __real__ _M_value = __z.real(); >- __imag__ _M_value = __z.imag(); >+ _M_value = __z.__rep(); These changes look OK, but I wonder if we shouldn't ask the compiler to make it possible to use __real__ and __imag__ in constexpr functions instead. I assume it doesn't, and that's why you made this change. But if it Just Worked, and the other changes I commented on above are also unnecessary, then this patch would *mostly* just be adding _GLIBCXX20_CONSTEXPR which is OK for stage 3 (as it doesn't affect any dialects except C++2a). >@@ -1872,7 +1831,7 @@ > { return _Tp(); } > > template<typename _Tp> >- inline typename __gnu_cxx::__promote<_Tp>::__type >+ _GLIBCXX_CONSTEXPR inline typename __gnu_cxx::__promote<_Tp>::__type This should be _GLIBCXX20_CONSTEXPR. >Index: testsuite/26_numerics/complex/comparison_operators/more_constexpr.cc >=================================================================== >--- testsuite/26_numerics/complex/comparison_operators/more_constexpr.cc (nonexistent) >+++ testsuite/26_numerics/complex/comparison_operators/more_constexpr.cc (working copy) >@@ -0,0 +1,51 @@ >+// { dg-do compile { target c++2a } } All the tests with { target c++2a} should also have: // { dg-options "-std=gnu++2a" } Because otherwise they are skipped by default, and only get run when RUNTESTFLAGS explicitly includes something like --target_board=unix/-std=gnu++2a The dg-options needs to come first, or it doesn't apply before the check for { target c++2a }. |
On 11/19/18 6:13 AM, Jonathan Wakely wrote:
> On 16/11/18 19:39 -0500, Ed Smith-Rowland wrote: >> @@ -322,67 +323,43 @@ >> //@{ >> /// Return new complex value @a x plus @a y. >> template<typename _Tp> >> - inline complex<_Tp> >> + inline _GLIBCXX20_CONSTEXPR complex<_Tp> >> operator+(const complex<_Tp>& __x, const complex<_Tp>& __y) >> - { >> - complex<_Tp> __r = __x; >> - __r += __y; >> - return __r; >> - } >> + { return complex<_Tp>(__x.real() + __y.real(), __x.imag() + >> __y.imag()); } > > Is this change (and all the similar ones) really needed? > > Doesn't the fact that all the constructors and member operators of > std::complex mean that the original definition is also valid in a > constexpr function? >> @@ -1163,50 +1143,43 @@ >> #endif >> >> template<typename _Tp> >> - complex& >> + _GLIBCXX20_CONSTEXPR complex& >> operator=(const complex<_Tp>& __z) >> { >> - __real__ _M_value = __z.real(); >> - __imag__ _M_value = __z.imag(); >> + _M_value = __z.__rep(); > > These changes look OK, but I wonder if we shouldn't ask the compiler > to make it possible to use __real__ and __imag__ in constexpr > functions instead. > > I assume it doesn't, and that's why you made this change. But if it > Just Worked, and the other changes I commented on above are also > unnecessary, then this patch would *mostly* just be adding > _GLIBCXX20_CONSTEXPR which is OK for stage 3 (as it doesn't affect any > dialects except C++2a). be better. Do you have any idea where this change would be? I grepped around a little and couldn't figure it out. if you don't I'll look more. Actually, looking at constexpr.c it looks like the old way ought to work... OK, plain assignment works but not the others. Interesting. > >> @@ -1872,7 +1831,7 @@ >> { return _Tp(); } >> >> template<typename _Tp> >> - inline typename __gnu_cxx::__promote<_Tp>::__type >> + _GLIBCXX_CONSTEXPR inline typename >> __gnu_cxx::__promote<_Tp>::__type > > This should be _GLIBCXX20_CONSTEXPR. >> Index: >> testsuite/26_numerics/complex/comparison_operators/more_constexpr.cc >> =================================================================== >> --- >> testsuite/26_numerics/complex/comparison_operators/more_constexpr.cc >> (nonexistent) >> +++ >> testsuite/26_numerics/complex/comparison_operators/more_constexpr.cc >> (working copy) >> @@ -0,0 +1,51 @@ >> +// { dg-do compile { target c++2a } } > > All the tests with { target c++2a} should also have: > > // { dg-options "-std=gnu++2a" } > > Because otherwise they are skipped by default, and only get run when > RUNTESTFLAGS explicitly includes something like > --target_board=unix/-std=gnu++2a > > The dg-options needs to come first, or it doesn't apply before the > check for { target c++2a }. > Updated patch attached. I'd like to understand why __real__ _M_value += __z.real(); doesn't work though. Ed patch_p0415_2 (46K) Download Attachment |
On 26/11/18 09:30 +0100, Christophe Lyon wrote:
>On Thu, 22 Nov 2018 at 10:20, Jonathan Wakely <[hidden email]> wrote: >> >> On 20/11/18 17:58 -0500, Ed Smith-Rowland wrote: >> >On 11/19/18 6:13 AM, Jonathan Wakely wrote: >> >>On 16/11/18 19:39 -0500, Ed Smith-Rowland wrote: >> >>>@@ -322,67 +323,43 @@ >> >>> //@{ >> >>> /// Return new complex value @a x plus @a y. >> >>> template<typename _Tp> >> >>>- inline complex<_Tp> >> >>>+ inline _GLIBCXX20_CONSTEXPR complex<_Tp> >> >>> operator+(const complex<_Tp>& __x, const complex<_Tp>& __y) >> >>>- { >> >>>- complex<_Tp> __r = __x; >> >>>- __r += __y; >> >>>- return __r; >> >>>- } >> >>>+ { return complex<_Tp>(__x.real() + __y.real(), __x.imag() + >> >>>__y.imag()); } >> >> >> >>Is this change (and all the similar ones) really needed? >> >> >> >>Doesn't the fact that all the constructors and member operators of >> >>std::complex mean that the original definition is also valid in a >> >>constexpr function? >> >These changes are rolled back. Sorry. >> >>>@@ -1163,50 +1143,43 @@ >> >>>#endif >> >>> >> >>> template<typename _Tp> >> >>>- complex& >> >>>+ _GLIBCXX20_CONSTEXPR complex& >> >>> operator=(const complex<_Tp>& __z) >> >>> { >> >>>- __real__ _M_value = __z.real(); >> >>>- __imag__ _M_value = __z.imag(); >> >>>+ _M_value = __z.__rep(); >> >> >> >>These changes look OK, but I wonder if we shouldn't ask the compiler >> >>to make it possible to use __real__ and __imag__ in constexpr >> >>functions instead. >> >> >> >>I assume it doesn't, and that's why you made this change. But if it >> >>Just Worked, and the other changes I commented on above are also >> >>unnecessary, then this patch would *mostly* just be adding >> >>_GLIBCXX20_CONSTEXPR which is OK for stage 3 (as it doesn't affect any >> >>dialects except C++2a). >> > >> >Yes, this is the issue. I agree that constexpr _real__, __imag__would >> >be better. >> > >> >Do you have any idea where this change would be? I grepped around a >> >little and couldn't figure it out. if you don't I'll look more. >> >> No idea, sorry. >> >> >Actually, looking at constexpr.c it looks like the old way ought to work... >> > >> >OK, plain assignment works but not the others. Interesting. >> > >> >> >> >>>@@ -1872,7 +1831,7 @@ >> >>> { return _Tp(); } >> >>> >> >>> template<typename _Tp> >> >>>- inline typename __gnu_cxx::__promote<_Tp>::__type >> >>>+ _GLIBCXX_CONSTEXPR inline typename >> >>>__gnu_cxx::__promote<_Tp>::__type >> >> >> >>This should be _GLIBCXX20_CONSTEXPR. >> >Done. >> >>>Index: >> >>>testsuite/26_numerics/complex/comparison_operators/more_constexpr.cc >> >>>=================================================================== >> >>>--- >> >>>testsuite/26_numerics/complex/comparison_operators/more_constexpr.cc >> >>>(nonexistent) >> >>>+++ >> >>>testsuite/26_numerics/complex/comparison_operators/more_constexpr.cc >> >>>(working copy) >> >>>@@ -0,0 +1,51 @@ >> >>>+// { dg-do compile { target c++2a } } >> >> >> >>All the tests with { target c++2a} should also have: >> >> >> >>// { dg-options "-std=gnu++2a" } >> >> >> >>Because otherwise they are skipped by default, and only get run when >> >>RUNTESTFLAGS explicitly includes something like >> >>--target_board=unix/-std=gnu++2a >> >> >> >>The dg-options needs to come first, or it doesn't apply before the >> >>check for { target c++2a }. >> >> >> >Thank you, done. >> >> OK for trunk, thanks. >> >> >Updated patch attached. I'd like to understand why >> > >> > __real__ _M_value += __z.real(); >> > >> >doesn't work though. >> >> Yes, I agree it should. If you don't figure it out please file a bug >> requesting that it works, so somebody else might look into it. >> > >Hi, >I have noticed that >FAIL: 26_numerics/complex/requirements/more_constexpr.cc >on arm and aarch64 >The error messages: >Excess errors: >/libstdc++-v3/testsuite/26_numerics/complex/requirements/more_constexpr.cc:168: >error: '__float128' was not declared in this scope >/libstdc++-v3/testsuite/26_numerics/complex/requirements/more_constexpr.cc:168: >error: no matching function for call to >'test_operator_members<<expression error>, __float128>()' >/libstdc++-v3/testsuite/26_numerics/complex/requirements/more_constexpr.cc:168: >error: template argument 1 is invalid patch.txt (1016 bytes) Download Attachment |
Free forum by Nabble | Edit this page |