[libc-commits] [libc] [libc][cmake] Unify unit and hermetic test rules into add_libc_test (PR #233512)

[Pavel Labath via libc-commits]

labath wrote:

> > * Could we reduce the amount of `if(TARGET)` checks? Particularly checks like `if(TARGET libc-tests)` seem completely superfluous as we create that target on the very first line of the cmake file.
> 
> Agreed — since `create_libc_test` returns early when `NOT LLVM_INCLUDE_TESTS` and `libc/test/CMakeLists.txt` unconditionally defines `libc-tests` and `libc-tests-build` at the very top, `if(TARGET libc-tests)` and `if(TARGET libc-tests-build)` (as well as `if(TARGET ${suite}-build)` in `_register_libc_test_with_suite`) were completely redundant. I removed them, and in a follow-up PR migrating all test directories to `add_libc_testsuite`, I will also remove the remaining `if(NOT TARGET ...)` lazy suite creation checks.

Sounds good. This looks much better. I'm looking forward to the followup

> 
> > * what is the advantage of moving the MPFR&MPC checks into the base add_libc_test function? It may make sense they are doing (or will be doing) something that can't be expressed via a wrapper, but that doesn't seem to be the case since add_libc_test pretty much just handles these two libraries and then forwards to the internal function. The advantage I see with the current setup (specialized wrappers) is that it scales better if we end up having more kinds of specialized tests in the future (we can handle that in the respective wrappers instead of putting everything in the base function).
> 
> You're right. My goal is to consolidate all the logic for target/test creations in few centralized functions, and all the various target types will just be trivial 1-line wrappers. I was initially taking it slowly step by step, function by function, but it does create a bit of awkward intermediate stages. So I just moved `FP_TEST` and `NEED_MPFR*`/`NEED_MPC` directly into `create_libc_test` to eliminate several extra temporaries, making `add_libc_unittest`, `add_libc_hermetic`, and `add_fp_unittest` all trivial wrappers.

That didn't exactly answer my question -- which more-or-less was: Why is handling MPFR et al. in `add_libc_test` (or some function called by it -- doesn't make a difference) better than having an `add_mpfr_test` wrapper around the other function?

That said, the arrangement here makes more sense to me today than it did the day before, and the removal of one layer of indirection has helped, so LGTM.


https://github.com/llvm/llvm-project/pull/233512