1577275 - with -std=gnu++17, build fails with NumberFormat.cpp:443:25: error: constexpr function never produces a constant expression [-Winvalid-constexpr]

Status: RESOLVED FIXED

(Core :: JavaScript: Internationalization API, defect, P2)

Description

[Nathan Froyd [:froydnj]]

Source code in question says:

static constexpr size_t MaxUnitLength() {
  // Enable by default when bug 1560664 is fixed.
#if __cplusplus >= 201703L
  size_t length = 0;
  for (const auto& unit : simpleMeasureUnits) {
    length = std::max(length, std::char_traits<char>::length(unit.subtype));
  }
  return length * 2 + std::char_traits<char>::length("-per-");
#else
...

I assume this is because simpleMeasureUnits is not constexpr? Or possibly because the standard library's std::max isn't constexpr?

Comment 1

[Jeff Walden [:Waldo]]

Attachment: Bug 1577275 - Make |simpleMeasureUnits| |static constexpr| to make some C++17-anticipating constexpr code work. r=anba

Comment 2

[Nathan Froyd [:froydnj]]

I am currently having to use:

@@ -445,7 +445,8 @@ static constexpr size_t MaxUnitLength() {
 #if __cplusplus >= 201703L
   size_t length = 0;
   for (const auto& unit : simpleMeasureUnits) {
-    length = std::max(length, std::char_traits<char>::length(unit.subtype));
+    const size_t subtype_length = std::char_traits<char>::length(unit.subtype);
+    length = length > subtype_length ? length : subtype_length;
   }
   return length * 2 + std::char_traits<char>::length("-per-");
 #else

to deal with not-constexpr std::max functions, FWIW.

Comment 3

[Jeff Walden [:Waldo]]

That's pretty strange -- std::max has been constexpr since C++14 per https://en.cppreference.com/w/cpp/algorithm/max and N3936 that's basically C++14 but free. Are we using a bad STL or something these days?

Comment 4

[Nathan Froyd [:froydnj]]

GCC 6.4+, at least, appear to properly declare things as constexpr.

I'm seeing this on an aarch64-linux cross-build, so maybe we don't have the correct headers there somehow?

Comment 5

[Nathan Froyd [:froydnj]]

I don't understand what's going on the the aarch64 cross build; the system libraries are GCC 6, which I have verified do understand how to do constexpr max().

Comment 6

[André Bargull [:anba]]

Hmm, interesting. https://godbolt.org/z/Lc7qOf works with just const in GCC >= 8; GCC < 8 or any Clang need constexpr. And std::char_traits<char>::length only works in constexpr contexts for GCC > 7.2.

Comment 7

[Jeff Walden [:Waldo]]

Whoops, I never landed this? Fixed now. *stabs lando for preventing him from regularly landing his own patches*

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f406c84f2bd9
Make |simpleMeasureUnits| |static constexpr| to make some C++17-anticipating constexpr code work.  r=anba

Comment 9

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/f406c84f2bd9