Re: [lmi] static constexpr

[Vadim Zeitlin]

On Wed, 16 Jun 2021 14:49:24 +0000 Greg Chicares <gchicares@sbcglobal.net> 
wrote:

GC> On 6/16/21 1:20 AM, Greg Chicares wrote:
GC> [...]
GC> > Tomorrow I'll reconsider the other occurrences of "static constexpr" to
GC> > see whether I can find any argument for retaining it anywhere.
GC> 
GC> Let's examine each case in
GC>   git grep 'static constexpr'
GC> in some detail. (Soon I'll commit a changeset and ask whether you
GC> agree that it's complete.)
GC> 
GC>   monnaie.hpp:    static constexpr int cents_digits = 2;
GC>   monnaie.hpp:    static constexpr int cents_per_dollar = 100; // 
std::pow(10, cents_digits)
GC>   monnaie.hpp:    static constexpr amount_type max_dollars()
GC>   currency.hpp:    static constexpr int    cents_digits     = 2;
GC>   currency.hpp:    static constexpr double cents_per_dollar = 100.0;
GC> 
GC> These are okay as they stand. They're class "properties" that can
GC> be queried by other code, even without a class instance.

 Yes, absolutely.

GC>   currency_test.cpp:    static constexpr double d0 = 
std::numeric_limits<double>::max();
GC>   currency_test.cpp:    static constexpr double d0 = 
std::numeric_limits<double>::infinity();
GC> 
GC> These are interesting. I'm quite sure you would remove 'static' here,
GC> for the same reason you'd remove it in the commit (c01b9b0) that
GC> started this discussion: we don't need storage allocated to hold the
GC> result. Here's the full context of the first instance:
GC> 
GC>   void currency_test::mete_humongous()
GC>   {
GC>       static constexpr double d0 = std::numeric_limits<double>::max();
GC>       static currency const extreme = from_cents(d0);
GC>       static currency const value   = from_cents(1234567);
GC>       for(int i = 0; i < 100000; ++i)
GC>           {
GC>           currency volatile z = std::min(extreme, value);
GC>           }
GC>   }
GC> 
GC> What I find interesting is that making all three local variables 'static'
GC> seemed like a good idea because it favors superficial consistency,

 I thought that an even better idea would be to make them all (non-static)
constexpr. Unfortunately this doesn't really work because this would
require making from_cents() itself constexpr, which also seems like an
excellent idea, but rint() is not constexpr in C++20 (this might change in
C++23, but doesn't help right now) and I don't know how to replace it with
something that could be evaluated at compile-time. We probably could steal
the generic implementation of remainder() from glibc, but I don't know if
it's worth doing it and it would probably be quite a bit slower during
run-time.

 FWIW I do think it would be at the very least worth changing this once
rint() becomes constexpr in the standard library because making
from_cents() constexper would detect any calls to it with incorrect
parameters at compile-time, which is much preferable to throwing an
exception during run-time (this is a special case of the benefits that
running constexpr-everything was supposed to provide) but I'm not sure if
it's worth spending time on this now, unless you see some simpler way of
testing for the condition "cents == std::rint(cents)" at compile-time than
implementing our own constexpr rint() replacement. I have a nagging feeling
that it should be possible, after all we don't need full rint() here, just
a way to check if the fractional part (but modf() is not constexpr neither
yet, of course) is 0, but I just don't see how to do it. All I can say is
that using "static_cast<int>(cents)" is out of question because this fails
(but at least at compile-time, which is nice) for any double outside of int
range.

 I also tried a pretty stupid version doing this:

---------------------------------- >8 --------------------------------------
#include <limits>

constexpr bool is_int(double d)
{
    if ( d < 0 )
        return is_int(-d);

    constexpr double maxint = static_cast<double>(std::numeric_limits<unsigned 
long long>::max());
    while ( d > maxint )
        d -= maxint;

    return d == static_cast<unsigned long long>(d);
}
---------------------------------- >8 --------------------------------------

and it does seem to work for "reasonable" inputs, but fails for d0 from the
test with

error: ‘constexpr’ loop iteration count exceeds limit of 262144 (use 
‘-fconstexpr-loop-limit=’ to increase the limit)

from gcc. And dividing by 2 inside the loop doesn't work because fractional
numbers become integers after just a few iterations due to the loss of
precision. So it seems that the only real compile-time solution is to
examine the bits of the exponent and fraction parts of the IEEE 754 double
value explicitly, which definitely looks like it would be possible but I'm
not sure if I trust myself to do it correctly right now.

GC> Testing with pc-linux-gnu only, I find that the time for mete_humongous(),
GC> with the first 'static' left intact, varies from one trial to the next;
GC> however, removing the first 'static' eliminates that variability, causing
GC> the reported time to be uniformly as the lowest timing without that change.

 I found this so surprising that I've examined assembler output from both
versions and, FWIW, I can confirm that gcc does allocate memory for d0
before your changes (i.e. with "static constexpr"), and doesn't do it with
just "constexpr", but I can't really explain how does it affect the speed
because it doesn't actually seem to use the value in memory anywhere, i.e.
the rest of the function code is exactly the same in both cases.

GC>   ihs_avsolve.cpp:        static constexpr currency C100 = 100_cents;
GC>   ihs_basicval.cpp:    static constexpr double dbl_inf = 
std::numeric_limits<double>::infinity();
GC>   ihs_basicval.cpp:    static constexpr double dbl_inf = 
std::numeric_limits<double>::infinity();
GC>   zero.hpp:    static constexpr double epsilon   
{std::numeric_limits<double>::epsilon()};
GC> 
GC> Those four should clearly be changed in light of the preceding discussion.

 Agreed.

GC>   view_ex.cpp://  static constexpr std::string unnameable{"Hastur"};
GC> 
GC> I thought gcc-10.2.0 might allow that, but no:
GC> 
GC> /opt/lmi/src/lmi/view_ex.cpp:213:34: error: the type ‘const string’ {aka 
‘const std::__cxx11::basic_string<char>’} of ‘constexpr’ variable ‘unnameable’ 
is not literal
GC>   213 |     static constexpr std::string unnameable{"Hastur"};
GC>       |                                  ^~~~~~~~~~
GC> 
/usr/lib/gcc/i686-w64-mingw32/10-win32/include/c++/bits/basic_string.h:77:11: 
note: ‘std::__cxx11::basic_string<char>’ is not literal because:
GC>    77 |     class basic_string
GC>       |           ^~~~~~~~~~~~
GC> 
/usr/lib/gcc/i686-w64-mingw32/10-win32/include/c++/bits/basic_string.h:77:11: 
note:   ‘std::__cxx11::basic_string<char>’ does not have ‘constexpr’ destructor
GC> 
GC> Maybe it'll work with a future gcc version.

 There are indeed plans to do it, AFAIR, but right now you have to write
your own constexpr string if you need it (as CTRE does).

GC> Until then, today's code:
GC>     static std::string const unnameable{"Hastur"};
GC> is plenty good enough.

 Yes, but it could still be replaced with constexpr string_view like this:

---------------------------------- >8 --------------------------------------
diff --git a/view_ex.cpp b/view_ex.cpp
index 0715d5478..0fd622e62 100644
--- a/view_ex.cpp
+++ b/view_ex.cpp
@@ -211,6 +211,6 @@ std::string ViewEx::base_filename() const
 // Allowed by
 //   http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2018/p0980r0.pdf
 //  static constexpr std::string unnameable{"Hastur"};
-    static std::string const unnameable{"Hastur"};
-    return path.has_filename() ? path.filename().string() : unnameable;
+    constexpr std::string_view unnameable{"Hastur"};
+    return path.has_filename() ? path.filename().string() : 
std::string{unnameable};
}
---------------------------------- >8 --------------------------------------

if you wanted (if you do, the comment above should be probably removed as
using a string_view is a perfectly cromulent workaround, I think).

GC>   stratified_algorithms.hpp:///   static constexpr T zero {};
GC>   stratified_algorithms.hpp:    static constexpr T zero {};
GC>   stratified_algorithms.hpp:    static constexpr T zero {};
GC>   stratified_algorithms.hpp:    static constexpr T zero {};
GC>   stratified_algorithms.hpp:    static constexpr T zero {};
GC>   stratified_algorithms.hpp:    static constexpr T zero {};
GC> 
GC> Those six cases are documented thus:
GC> 
GC>   /// Implementation note: Many function templates in this header
GC>   /// require a zero of the appropriate type T, and accordingly define
GC>   /// a local variable
GC>   ///   static constexpr T zero {};
GC>   /// in case this zero is expensive to construct. It is constructed
GC>   /// as "T{}" rather than "T(0)" because the latter uses an explicit
GC>   /// integer argument, which may require a converting constructor
GC>   /// (for example, with class currency).
GC> 
GC> so 'static constexpr' seems appropriate: the constant may be
GC> expensive to construct, so we want to construct it OAOO and
GC> store its value.

 Sorry, this is the (only) one change that I disagree with. Making it
static doesn't help help at all with constructing it once compared to
making it just constexpr: in the latter case it's constructed once at
compile-time, in the latter case it's constructed at compile-time but also
appears in the executable during run-time, which is needless. I.e.
"constexpr" alone is sufficient to construct it OAOO at compile-time and
while "static" would indeed be needed to store its value, we just don't
need to do this at all.

 Please let me know if I'm missing something here, but this seems exactly
the same as the other cases that you've changed in 1c7d29114 (Use 'static'
with 'constexpr' only for good reason, 2021-06-16).

 Thanks,
VZ

pgptIMe2Vy3uM.pgp
Description: PGP signature