[libcxx-commits] [PATCH] D93815: [libc++] [P1065] Constexpr invoke, reference_wrapper, mem_fn, not_fn.

[Arthur O'Dwyer via Phabricator via libcxx-commits]

Quuxplusone marked an inline comment as done.
Quuxplusone added inline comments.


================
Comment at: libcxx/test/std/utilities/function.objects/func.invoke/invoke_constexpr.pass.cpp:269
+
+int main(int, char**) {
+    static_assert(bullet_one_two_tests());
----------------
zoecarver wrote:
> I assume it didn't make sense to merge these with the existing tests because of functions like `foo` which would only work in one mode or the other? 
> 
> Is there any test coverage here that isn't in the non-constexpr version? If so, would it make sense to run these tests outside of a constexpr context as well (it very well might not)?
A mix of that reason, and the reason that I wanted to make sure constexprifying things didn't somehow //break// the non-constexpr codepath. One thing we haven't been worrying about much with these patches, and which I suddenly decided to worry about for this patch (why? I don't know) is that when we mark all the helper/component functions like `foo` with `TEST_CONSTEXPR_CXX20`, we are //losing// test coverage of how the thing works with non-constexpr functions. For `invoke`, that suddenly seemed important to test.

There is no test coverage here that isn't in the non-constexpr version. I suppose it wouldn't hurt to run these tests also outside of constexpr; I'll add that.


================
Comment at: libcxx/test/std/utilities/function.objects/func.not_fn/not_fn.pass.cpp:548
     { // test multi arg
+        using String = const char *;
         const double y = 3.14;
----------------
zoecarver wrote:
> This is just because `std::string` isn't constexpr?
Correct. The `obj` it's calling is actually callable with `auto...`, so it doesn't matter what types we pass to it. I don't know the original author's motivation for using `std::string` here; probably just it seemed convenient at the time.

I considered removing //all// this file's usage of `<string>`, but it seemed like an unnecessary rabbit hole at the moment.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93815