Re: [PATCH] Fix algo constexpr tests in Debug mode

[François Dumont <frs dot dumont at gmail dot com>]

On 9/30/19 11:03 AM, Jonathan Wakely wrote:
> On 28/09/19 23:12 +0200, François Dumont wrote:
> > Here is what I just commited.
>
> Thanks. In my branch I had a lot more _GLIBCXX20_CONSTEXPR additions
> in the debug mode headers. Is it not needed on __check_singular,
> __foreign_iterator etc. ?

Yes, I know, I just limited myself to fixing testsuite tests for the 
moment. I'll check if those need it too.

> > I try to use the asm trick in the _GLIBCXX_DEBUG_VERIFY_COND_AT but 
> > didn't notice any enhancement. So for now I kept my solution to just 
> > have a non-constexpr call compiler error.
>
> You won't see any improvement in the testsuite, because the compiler
> flags for the testsuite suppress diagnostic notes. But I'd expect it
> to give better output for users with the default compiler flags.
Ok, I didn't know, I'll give it another try then outside testsuite.
> > diff --git a/libstdc++-v3/include/bits/stl_algo.h 
> > b/libstdc++-v3/include/bits/stl_algo.h
> > index a672f8b2b39..f25b8b76df6 100644
> > --- a/libstdc++-v3/include/bits/stl_algo.h
> > +++ b/libstdc++-v3/include/bits/stl_algo.h
> > @@ -5054,8 +5054,8 @@ _GLIBCXX_BEGIN_NAMESPACE_ALGO
> >    *  @param  __last1   Another iterator.
> >    *  @param  __last2   Another iterator.
> >    *  @param  __result  An iterator pointing to the end of the merged 
> > range.
> > -   *  @return         An iterator pointing to the first element 
> > <em>not less
> > -   *                  than</em> @e val.
> > +   *  @return   An output iterator equal to @p __result + (__last1 - 
> > __first1)
> > +   *            + (__last2 - __first2).
> >    *
> >    *  Merges the ranges @p [__first1,__last1) and @p 
> > [__first2,__last2) into
> >    *  the sorted range @p [__result, __result + (__last1-__first1) +
> > @@ -5102,8 +5102,8 @@ _GLIBCXX_BEGIN_NAMESPACE_ALGO
> >    *  @param  __last2   Another iterator.
> >    *  @param  __result  An iterator pointing to the end of the merged 
> > range.
> >    *  @param  __comp    A functor to use for comparisons.
> > -   *  @return         An iterator pointing to the first element "not 
> > less
> > -   *                  than" @e val.
> > +   *  @return   An output iterator equal to @p __result + (__last1 - 
> > __first1)
> > +   *            + (__last2 - __first2).
> >    *
> >    *  Merges the ranges @p [__first1,__last1) and @p 
> > [__first2,__last2) into
> >    *  the sorted range @p [__result, __result + (__last1-__first1) +
>
> These changes are fine but should have been a separate, unrelated
> commit.

Ok, sorry, I consider that just a comment change was fine.


> > @@ -157,10 +192,16 @@ namespace __gnu_debug
> >    *  otherwise.
> >   */
> >   template<typename _InputIterator>
> > +    _GLIBCXX20_CONSTEXPR
> >     inline bool
> >     __valid_range(_InputIterator __first, _InputIterator __last,
> >           typename _Distance_traits<_InputIterator>::__type& __dist)
> >     {
> > +#ifdef __cpp_lib_is_constant_evaluated
> > +      if (std::is_constant_evaluated())
> > +    // Detected by the compiler directly.
> > +    return true;
> > +#endif
>
> Should this be using the built-in, not the C++20 function?
>
>
> In practice it's probably equivalent, because the function is only
> going to be constant-evaluated when called from C++20 code, and in
> that case the std::is_constant_evaluated() function will be available.


Yes, this is why I did it this way. And moreover it is using std::pair 
move assignment operator which is also C++20 constexpr.

> It just seems inconsistent to use the built-in in one place and not in
> the other.

It is also the reason why the simple simple __valid_range is not using 
the other anymore.

Maybe once I'll have check all the algo calls I'll find out that this 
one need _GLIBCXX_CONSTEXPR.

I got the sensation that library is being 'constexpr' decorated only 
when needed and when properly Standardise so are the Debug helpers.

François