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

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

Quuxplusone added inline 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
----------------
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.")


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116388