Re: [lmi] std::identity unavailable in clang libc++

[Vadim Zeitlin]

On Wed, 14 Jul 2021 02:11:02 +0000 Greg Chicares <gchicares@sbcglobal.net> 
wrote:

GC> On 2021-07-05 13:02, Vadim Zeitlin wrote:
GC> > 
GC> >  Unfortunately the perfectly innocent changes of 28955d101 (Refactor for
GC> > flexibility, 2021-07-03) broke clang build due to lack of std::identity in
GC> > its standard library, see
GC> > 
GC> > 
https://github.com/let-me-illustrate/lmi/runs/2985604779?check_suite_focus=true
GC> > 
GC> >  The compiler currently used for the clang builds is clang 11, but this
GC> > metafunction is still not implemented in clang 12 neither, so upgrading it
GC> > wouldn't help. Testing for __cpp_lib_type_identity would help to check
GC> > whether it's available or not, but this is just part of the solution
GC> > because we'd still need to do something to allow lmi_root() declaration to
GC> > compile.
GC> 
GC> There are two "identity" things:
GC>   - std::identity      // <functional>   [func.identity]
GC>   - std::type_identity // <type_traits>  [meta.trans.other]
GC> AFAICT __cpp_lib_type_identity tests for the latter, but we want the former,

 Oops, sorry, I've been hopelessly confused and indeed mixed up the two.

GC> which doesn't seem to have a feature-test macro

 No, it doesn't. Or at least I couldn't find anything appropriate at
https://en.cppreference.com/w/cpp/utility/feature_test nor in the output of
"echo '#include <version>' | g++-11 -std=c++20 -dM -E -x c++ -".

GC> ...or is one of the following suitable?
GC> 
GC>   __cpp_lib_constexpr_functional
GC> because it uses 'constexpr' in [func.identity]
GC> 
GC>   __cpp_lib_transparent_operators
GC> because it uses 'is_transparent' in [func.identity]
GC> 
GC> Or maybe both should be used; or maybe even both together aren't enough.

 Both together are not enough because clang 11 and 12 define both of them,
but don't provide std::identity().

GC> >  I see 2 possibilities here:
GC> > 
GC> > 1. Define and use lmi_identity.
GC> >  (a) Either just define it trivially as shown in the working paper for
GC> >      std::identity itself: 
http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2018/p0887r0.pdf
GC> 
GC> Doesn't that paper pertain to std::type_identity?

 Again, it does, sorry for mixing them up. Searching for "identity" is
complicated...

GC> Instead, see this very recent commit:
GC>   [master 4d8cb2292] Work around a clang issue

 This does work with clang, thanks!

GC> >  (b) Or define it like this unless __cpp_lib_type_identity is defined, in
GC> >      which case alias it to std::identity itself.
GC> > 
GC> > 2. Define std::identity in case __cpp_lib_type_identity is not defined.
GC> 
GC> There doesn't seem to exist a feature-test macro for this.

 Yes, the only way to test for it would be to use the old approach of
testing for the compilers, and compiler versions, with support for it. I
could write the necessary checks but it is going to be quite a bit uglier
than just checking for __cpp_lib_something.

GC> >  The obvious problem of (2) is that we are not allowed to do this, but 
OTOH
GC> > I can't see what problems could this ever cause this in practice -- and 
the
GC> > code doing it, inside "#ifndef __cpp_lib_type_identity", wouldn't ever be
GC> > used for production builds using gcc anyhow. So personally I'd rather do
GC> > (2) and keep lmi_root() itself unchanged. Would you agree with such hack?
GC> 
GC> I'd have preferred that, yes. What I did is arguably more hackish.

 Please let me know if you'd like me to make a patch with such checks.

GC> I wanted to put it someplace like
GC>   math_functions.hpp [but it's not a math function]
GC>   stl_extensions.hpp [but it's not STL]
GC>   miscellany.hpp     [which is already too...miscellaneous]
GC> but, it being a hapax legomenon, I hacked it locally, where it'll
GC> be easy to revert.

 Let's just hope it remains unique...

GC> Now let's see whether clang has std::midpoint() [commit facecf48b].

 It does, but its CI build still fails due to the following warnings/errors
(werrors?), that I hadn't previously noticed because of the other error:

---------------------------------- >8 --------------------------------------
financial_test.cpp:74:37: error: variable 'unoptimizable' is uninitialized when 
passed as a const reference argument here 
[-Werror,-Wuninitialized-const-reference]
    stifle_warning_for_unused_value(unoptimizable);
                                    ^~~~~~~~~~~~~
financial_test.cpp:96:37: error: variable 'unoptimizable' is uninitialized when 
passed as a const reference argument here 
[-Werror,-Wuninitialized-const-reference]
    stifle_warning_for_unused_value(unoptimizable);
                                    ^~~~~~~~~~~~~
---------------------------------- >8 --------------------------------------

 We have already had the same problem recently and so I think it should be
fixed in the same way it was done in 0380e928c (Move
stifle_warning_for_unused_value() after setting the value, 2021-04-28), see
https://lists.nongnu.org/archive/html/lmi/2021-04/msg00058.html, i.e. just

---------------------------------- >8 --------------------------------------
diff --git a/financial_test.cpp b/financial_test.cpp
index fa0b16775..0324a57a0 100644
--- a/financial_test.cpp
+++ b/financial_test.cpp
@@ -71,7 +71,6 @@
     constexpr int decimals {5};
     static std::vector<double> results(payments.size());
     volatile double unoptimizable;
-    stifle_warning_for_unused_value(unoptimizable);
     for(int i = 0; i < 10; ++i)
         {
         irr
@@ -83,6 +82,7 @@
             );
         unoptimizable = results.front();
         }
+    stifle_warning_for_unused_value(unoptimizable);
 }

 void mete_1
@@ -93,7 +93,6 @@
     constexpr int decimals {5};
     static std::vector<double> results(payments.size());
     volatile double unoptimizable;
-    stifle_warning_for_unused_value(unoptimizable);
     for(int i = 0; i < 10; ++i)
         {
         irr
@@ -106,6 +105,7 @@
             );
         unoptimizable = results.front();
         }
+    stifle_warning_for_unused_value(unoptimizable);
 }

 int test_main(int, char*[])
---------------------------------- >8 --------------------------------------

 If you'd like, I could commit this directly, with a similar commit
message, or, if you prefer, you could do it yourself, just let me know.

 And thanks again for fixing the std::identity problem!
VZ

pgpBW8WiNDI3T.pgp
Description: PGP signature