[libcxx-commits] [PATCH] D106798: [libc++] [test] Add support for `ADDITIONAL_COMPILE_FLAGS_IF_SUPPORTED` directive.

Thu Jul 29 00:25:40 PDT 2021

curdeius added a comment.

In D106798#2911473 <https://reviews.llvm.org/D106798#2911473>, @ldionne wrote:

> In D106798#2911091 <https://reviews.llvm.org/D106798#2911091>, @curdeius wrote:
>
>> In D106798#2910503 <https://reviews.llvm.org/D106798#2910503>, @ldionne wrote:
>>
>>> In D106798#2904406 <https://reviews.llvm.org/D106798#2904406>, @curdeius wrote:
>>>
>>>> @ldionne, is this acceptable or is it too hackish?
>>>
>>> So this is to add `-fconstexpr-steps=N`, correct? Clang supports that flag, GCC doesn't. Does GCC have a sufficiently large number of constexpr steps by default?
>>
>> Exactly. GCC is okay by default.
>
> That would just be side-stepping the problem, but perhaps that means that Clang's default limit on constexpr steps is too low? What do you think?

I'm not sure if it's the way to go. Clang has already a limit of 1Mi (`1<<20`), which seems reasonably high for normal code.
However, GCC has higher limits, `-fconstexpr-loop-limit=n` is `1<<18` and `-fconstexpr-ops-limit=n` (equivalent of `-fconstexpr-steps=n`) is `1<<25` so about 32Mi.
Also, changing the limit in clang will mean that we need to wait like 1 year to be able to use it in all supported versions of clang.

Another option, not sure if there's anything similar there already, having a directive that will map to the appropriate compiler option.
E.g. `// MAX_CONSTEXPR_STEPS: 123456` would map to `-fconstexpr-steps=123456` on Clang and `-fconstexpr-ops-limit=123456` on GCC, and (possibly) `/fconstexpr:steps123456` on MSVC.
It could be probably made more generic to just have one directive for many flags. E.g. `// MAP_COMPILE_FLAGS: constexpr-steps=123456, other-flag=something`.
The advantage would be to error out on unsupported flags.
In comparison, `ADDITIONAL_COMPILE_FLAGS_IF_SUPPORTED` would just ignore e.g. a flag with a typo silently on all compilers.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106798