[libcxx-commits] [PATCH] D116388: [libc++] Properly handle specializations of std::is_placeholder.

[Louis Dionne via Phabricator via libcxx-commits]

ldionne requested changes to this revision.
ldionne added a comment.
This revision now requires changes to proceed.

Do I understand correctly that we are technically conforming without this change, however this resolves implementation divergence? If so, let's create a LWG issue just to make sure the Standard actually describes what all implementations are doing.

Side note: The preferred way of referring to bugs in the code base should be `http://llvm.org/PR51753`. Unfortunately, that won't cause GitHub to automatically close issues when the fix is committed, which is kind of annoying. I'm on the fence as to what we should do about this, but I guess for now it's fine to use either (but let's use `llvm.org/PRXXXXX` inside code comments).



================
Comment at: libcxx/test/std/utilities/function.objects/bind/func.bind/func.bind.isbind/is_bind_expression.pass.cpp:26
+    static_assert(std::is_bind_expression<const T>::value == Expected, "");
+    static_assert(std::is_bind_expression<const T&>::value == Expected, "");
+#endif
----------------
Quuxplusone wrote:
> jloser wrote:
> > Since the library code does `__uncvref_t`, should we also test `volatile T` here and below? Or is your thought "down with volatile in the library!" basically? :)
> Basically the latter, yeah. ;)
> I mean, suppose something here didn't work properly with `volatile T`: I'd still call that a bug and want to fix it (and regression-test it at that point). But I have the vague impression that the paper standard might //not// consider that a bug — //does// library stuff need to work with `volatile`, or does `volatile` void the warranty in the same way as a throwing destructor? (I'm not sure.)
> Plus, ideally `volatile`-specific bugs should be extremely rare, because most of what the library does with cv-qualifiers is colorblind as to whether it's C or V, right? Like, in this particular PR we're doing `__uncvref_t` which strips const and volatile symmetrically. I idealistically hope that most library code is like this, so testing `const` should get you most of the benefits of testing both, //even if// `const` bugs and `volatile` bugs were equally likely in the wild (which as you point out, they're not, because nobody should be using `volatile` like this). (Counterpoint: obviously the //language// does not treat `const` and `volatile` perfectly symmetrically: there's lots of `const T&` in the library and no `volatile T&`.)
> Bottom line: Is it worth doubling the length of every test in order to preempt volatile-specific bugs? (I'm very much convinced that the answer is "no.")
Agreed here. We should test `volatile` when we have `volatile`-specific behavior or when we fix an actual volatile-related bug (so we add a regression test), but otherwise I'd avoid adding complexity just for that qualifier.


================
Comment at: libcxx/test/std/utilities/function.objects/bind/func.bind/func.bind.isbind/is_placeholder.pass.cpp:22-24
+    static_assert(std::is_placeholder<T&>::value == Expected, "");
+    static_assert(std::is_placeholder<const T>::value == Expected, "");
+    static_assert(std::is_placeholder<const T&>::value == Expected, "");
----------------
`LIBCPP_STATIC_ASSERT` instead?

Also, this should have been `_LIBCPP_VERSION` instead -- I guess that's one additional reason to use `LIBCPP_STATIC_ASSERT`.


================
Comment at: libcxx/test/std/utilities/function.objects/bind/func.bind/func.bind.isbind/specialization.pass.cpp:17
+//   be treated as a subexpression in a bind call.
+//   https://github.com/llvm/llvm-project/issues/51095
+
----------------



================
Comment at: libcxx/test/std/utilities/function.objects/bind/func.bind/func.bind.isplace/specialization.pass.cpp:17
+//   to indicate that T should be treated as a placeholder type.
+//   https://github.com/llvm/llvm-project/issues/51095
+
----------------



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116388