[libcxx-commits] [PATCH] D110005: [WIP][CMake] Unify variable names

Thu Mar 24 09:24:14 PDT 2022

phosek added a comment.

In D110005#3405081 <https://reviews.llvm.org/D110005#3405081>, @mstorsjo wrote:

> Rebased to current git main. I split out the parts concerning `-std=c11` in compiler-rt, as those can be handled differently, separately from this.
>
> I also split out rewriting of the variable name for the `-ffreestanding` flag in compiler-rt, as that test doesn't seem to be shared with any of the other runtimes.
>
> So far, I think the scope of the patch was to rename all of the variable names within libcxx/libcxxabi/libunwind, and the ones in compiler-rt that overlaps any of them, but there's lots of more variables in compiler-rt that could be renamed consistently, even if it doesn't add any extra cache sharing across projects.

Thanks for taking over, I was far too busy over the last few weeks, but I should have more time for the next few weeks if you need help.

In D110005#3405107 <https://reviews.llvm.org/D110005#3405107>, @mstorsjo wrote:

> I guess the naming of variables was indeed the only main issue to settle here. The currently proposed naming in this patch originally, e.g. `CXX_SUPPORTS_...`, does match what LLVM uses in `add_flag_if_supported` here: https://github.com/llvm/llvm-project/blob/main/llvm/cmake/modules/HandleLLVMOptions.cmake#L260-L265
>
> So in this current form, this patch can also get cache hits between tests from llvm/cmake/modules and the rest of the runtimes, while if we settle on a different naming scheme like `CXX_COMPILER_SUPPORTS_...` we won't get  matches between tests in those different areas. (Not sure if there are any right now, but there could be.) And expanding this patch to rename the naming style of LLVM's cmake modules would grow the scope of this patch quite significantly.
>
> Or what's @beanz 's opinion on it?

Right, that's why I chose the naming scheme in the original patch, to avoid having to rename variables in `llvm/cmake/modules` and reduce the churn.

I think it's doable, but if we decide that's the direction we want to take, then I'd do it as a separate change .

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110005