[libcxx-commits] [PATCH] D131836: [libc++][CI] increases constexpr evaluation limit.

[Louis Dionne via Phabricator via libcxx-commits]

ldionne requested changes to this revision.
ldionne added a comment.
This revision now requires changes to proceed.

I hate to rain on the parade, but I don't think this patch is desirable as-is. Indeed, we want to make sure that we compile our test suite with flags as similar as possible to what the users use. Otherwise, you end up not testing what matters, i.e. what users do.

For example, if we add `-fconstexpr-steps=10000000` to the test suite, we could easily introduce a constexpr algorithm that always busts the default Clang limit even for small inputs, and our test suite would never catch it. Users would try to use the algorithm with a trivial example and things wouldn't work. That would be rather embarrassing.

So any approach where we turn this on for the whole test suite is a no-go IMO. Instead, we should increase the limit only for tests that actually require it, to reduce our exposure to the above bugs. One way to do it would be:

  // In a test that requires more constexpr steps:
  // ADDITIONAL_COMPILE_FLAGS(has-fconstexpr-steps): -fconstexpr-steps=100000
  // ADDITIONAL_COMPILE_FLAGS(has-fconstexpr-ops-limit): -fconstexpr-ops-limit=100000

Then, in `features.py`:

  Feature(name='has-fconstexpr-steps', when=lambda cfg: hasCompileFlag(cfg, '-fconstexpr-steps=1')),
  Feature(name='has-fconstexpr-ops-limit', when=lambda cfg: hasCompileFlag(cfg, '-fconstexpr-ops-limit=1')),

I just need to implement support for this new fancy form of `ADDITIONAL_COMPILE_FLAGS`. I'll look into it immediately, this would be useful on its own even regardless of this review.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131836/new/

https://reviews.llvm.org/D131836