Re: [lmi] New clang errors fixes

[Vadim Zeitlin]

On Tue, 26 Sep 2017 17:02:58 +0000 Greg Chicares <address@hidden> wrote:

GC> On 2017-09-17 17:35, Vadim Zeitlin wrote:
GC> > 
GC> >  I tried to build the latest sources with clang, as part of my tests, and
GC> > it turns out that there are 2 new errors: one due to the use of
GC> > non-constexpr std::ldexp() in the initialization of a constexpr variable 
in
GC> > bourn_cast.hpp and another one (but occurring thrice) due to the use of
GC> > deprecated std::auto_ptr<> in boost/regex.hpp.
GC> > 
GC> >  I've fixed both of them in https://github.com/vadz/lmi/pull/59
GC> 
GC> Let's consider std::ldexp() first:
GC> 
GC> 
https://github.com/vadz/lmi/pull/59/commits/72b38891924db39a5b07cec56b4e39e9a4414875
GC> 
GC> -    constexpr From limit = std::ldexp(From(1), to_traits::digits);
GC> +    static From const limit = std::ldexp(From(1), to_traits::digits);
GC> 
GC> As the commit message remarks, that partially reverts this earlier change:
GC> 
GC> https://github.com/vadz/lmi/commit/15edd3b9f8a1847252a3a38ab29796104da3253b
GC> 
GC> -    static From const limit = std::ldexp(From(1), to_traits::digits);
GC> +    constexpr From limit = std::ldexp(From(1), to_traits::digits);
GC> 
GC> whose commit message refers to this post:
GC>   http://lists.nongnu.org/archive/html/lmi/2017-08/msg00068.html
GC> which says that change was needed for gcc-6.3.0 and -std=c++17 as used
GC> for production currently.

 Oops, sorry, I completely forgot about this.

GC> If it can't be static and it can't be constexpr, then is this:
GC> +    From const limit = std::ldexp(From(1), to_traits::digits);
GC> the best we can do to express the intention and help the compiler optimize?

 I don't know if it's the best, but I wonder if this is really noticeably
worse than the best. I might be forgetting this as well, but have you
already run the benchmarks for this variant and did it show that it's
really worth it to optimize this call to ldexp() away?

GC> Or should we write our own function, e.g.:
GC> 
GC> -    constexpr From limit = std::ldexp(From(1), to_traits::digits);
GC> +    constexpr From limit = const_ldexp(From(1), to_traits::digits);
GC> 
GC> ? All we're trying to do is raise two to a representable power. Is there
GC> a clean way to do that within the intersection of both compilers' constexpr
GC> rules (and perhaps msvc's too, if reasonable)? Iteration? Recursion? Table
GC> lookup? Bitwise manipulation of an array of sizeof(long double) bits, with
GC> reinterpret_cast<long double>?

 The last one is an especially interesting idea, but there is a slightly
simpler way to write this function with C++17 (this wasn't valid code in
C++14 and earlier, so your decision to switch to it was very premonitory):

---------------------------------- >8 --------------------------------------
template<typename T>
constexpr T ldexp(T x, int exp)
{
    for(int i = 0; i < exp; ++i)
        {
        x *= 2;
        }

    return x;
}

int main()
{
    constexpr auto x = ldexp(1, 7);
    return x;
}
---------------------------------- >8 --------------------------------------

Compiling and running this program exits with error code 128 and its entire
code is:

---------------------------------- >8 --------------------------------------
(gdb) disassemble
Dump of assembler code for function main:
=> 0x0000555555554510 <+0>:     mov    eax,0x80
   0x0000555555554515 <+5>:     ret
End of assembler dump.
---------------------------------- >8 --------------------------------------

 So recursion is not the one true way any more, unless you prefer
stress-testing your compiler.

 The only problem with this approach is that MSVS 2015 that I still use
doesn't support this yet. However MSVS 2017 does, so perhaps this will
finally push me to update to it. But it seems a pity to write the function
in a less clear recursive way IMHO.

 However I can't guarantee that this variant avoids the problem in the unit
test you've encountered, as it seems like a compiler bug to me. But it
could be worth at least trying it.


GC> As for std::auto_ptr<>, I guess we're stuck with it as long as we depend
GC> on boost headers that use it. We could insert pragmata into our code
GC> directly wherever needed, as is done here.
GC>   
https://github.com/vadz/lmi/pull/59/commits/f799027a247dbd30e8eddf3e9d5804e7b8064c4d
GC> or...
GC> 
GC> > Alternatively, we could have a wrapper header for boost/regex.hpp, but 
this
GC> > doesn't seem useful from a long term perspective as we know that we will
GC> > replace it with std::regex sooner or later
GC> 
GC> I like that idea. That we'll eventually replace it strikes me as an
GC> argument in favor of this approach: it comes with its own disposal bag,
GC> as it were, so that we can more easily and cleanly discard it later
GC> instead of spending energy to find and remove every pragma.

 OK, I'll do this then. But, for completeness, another possibility, which
is a slightly less awful way of doing this:

GC> BTW, I also considered:
GC>   #define auto_ptr unique_ptr
GC> but it's hard to guess what risks that might entail.

would be to just replace auto_ptr with unique_ptr in the Boost headers we
use. It's not like we're ever going to update them anyhow...

 Regards,
VZ