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

[Mark de Wever via Phabricator via libcxx-commits]

Mordante marked 2 inline comments as done.
Mordante added a comment.

Thanks for the reviews!



================
Comment at: libcxx/utils/libcxx/test/features.py:42-51
+  # The default maximum number of operations in a constexpr function are too
+  # small for some tests. A "correct" value has been determined empirical.
+  Feature(name='clang-constexpr-steps',
+          when=lambda cfg: hasCompileFlag(cfg, '-fconstexpr-steps=1'),
+          actions=[AddCompileFlag('-fconstexpr-steps=128000000')]),
+
+  Feature(name='gcc-constexpr-steps',
----------------
philnik wrote:
> Is there a reason we don't want this in `params.py`? I can't really see a case where we want to restrict tests based  on `clang-constexpr-steps` or `gcc-constexpr-steps`.
I'm not entirely sure what you mean. `params.py` is the file with the user configurable settings. I don't want the user to change this setting. What am I missing?


================
Comment at: libcxx/utils/libcxx/test/features.py:43
+  # The default maximum number of operations in a constexpr function are too
+  # small for some tests. A "correct" value has been determined empirical.
+  Feature(name='clang-constexpr-steps',
----------------
var-const wrote:
> var-const wrote:
> > Ultranit: `s/empirical/empirically/`.
> I think it would be helpful to write the default value for comparison (I think it's `1048576` for Clang -- see `clang/include/clang/Basic/LangOptions.def`).
I don' think that's really helpful, Clang may change this at some point and then the comment is out of date. When really wanted to have the number I'd rather link to the source of truth.


================
Comment at: libcxx/utils/libcxx/test/features.py:45
+  Feature(name='clang-constexpr-steps',
+          when=lambda cfg: hasCompileFlag(cfg, '-fconstexpr-steps=1'),
+          actions=[AddCompileFlag('-fconstexpr-steps=128000000')]),
----------------
var-const wrote:
> This flag is not supported by GCC. Does this configuration know not to pass the flag to GCC (and same for the GCC flag)?
Yes that's done by the `hasCompileFlag` part, when not supported it isn't added.


================
Comment at: libcxx/utils/libcxx/test/features.py:46
+          when=lambda cfg: hasCompileFlag(cfg, '-fconstexpr-steps=1'),
+          actions=[AddCompileFlag('-fconstexpr-steps=128000000')]),
+
----------------
philnik wrote:
> Are we sure we want to bring this right to the maximum? This could increase the built time by 100x when you actually have bad code.
I can remove one 0 and it still works, but I like to have a bit of a margin. Lowering it further fails in the test.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131836