Closed Bug 1577275 Opened 3 months ago Closed 2 months ago

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

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla71
Tracking Status
firefox71 --- fixed

People

(Reporter: froydnj, Assigned: Waldo)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

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?

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.

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?

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?

Depends on: 1578535

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().

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.

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

Priority: -- → P2
Pushed by jwalden@mit.edu:
https://hg.mozilla.org/integration/autoland/rev/f406c84f2bd9
Make |simpleMeasureUnits| |static constexpr| to make some C++17-anticipating constexpr code work.  r=anba
Status: NEW → RESOLVED
Closed: 2 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla71
Assignee: nobody → jwalden
You need to log in before you can comment on or make changes to this bug.