[libcxx-commits] [PATCH] D127760: [libc++][NFC] Merges unused functions in callers.

[Joe Loser via Phabricator via libcxx-commits]

jloser added inline comments.


================
Comment at: libcxx/test/libcxx/numerics/bit.ops.pass.cpp:34
-    static_assert( std::__libcpp_popcount(v) == 7, "");
-    static_assert( std::__bit_log2(v) == 12, "");
-    static_assert(!std::__has_single_bit(v), "");
----------------
Mordante wrote:
> jloser wrote:
> > This is a drop in test coverage - I think we should keep this one for `__bit_log2`. In general, I think we should:
> > - Keep existing test coverage for the private functions kept (i.e. ones that had two or more call sites used by public functions).
> > - Verify existing coverage for public functions where we removed the private function it delegated to if we removed the private function (since it only had one call site). 
> > 
> > How does that sound? If you're up for it, we could go one step further and verify the `noexcept` properties.
> I disagree it's a drop in test coverage. The function is indirectly tested by its callers.
> But I don't mind to restore the test, so I'll add it back.
> 
> This is the only function "dropped" the other two remaining functions are still tested.
> There is test coverage for the public functions, I didn't verify all, but I trust our contributors and reviewers. These tests are more detailed than the sanity checks in this file. For example `test/std/numerics/bit/bitops.count/countl_one.pass.cpp`.
> 
> I think it's useful to validate the `noexcept` status when it's not mandated by the Standard.
> 
> 
Valid point.  It is indirectly tested with `bit_floor` and `bit_width`, though I do like the direct testing of it - thanks for adding that back.

Appreciate the added tests in general - LGTM!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127760