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

[Jonathan Wakely <jwakely@redhat.com>]

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. ?

>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.


>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.


>@@ -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.
It just seems inconsistent to use the built-in in one place and not in
the other.

>       typedef typename std::__is_integer<_InputIterator>::__type _Integral;
>       return __valid_range_aux(__first, __last, __dist, _Integral());
>     }
>@@ -180,11 +221,17 @@ namespace __gnu_debug
> #endif
> 
>   template<typename _InputIterator>
>+    _GLIBCXX_CONSTEXPR
>     inline bool
>     __valid_range(_InputIterator __first, _InputIterator __last)
>     {
>-      typename _Distance_traits<_InputIterator>::__type __dist;
>-      return __valid_range(__first, __last, __dist);
>+#ifdef _GLIBCXX_HAVE_BUILTIN_IS_CONSTANT_EVALUATED
>+      if (__builtin_is_constant_evaluated())
>+	// Detected by the compiler directly.
>+	return true;
>+#endif
>+      typedef typename std::__is_integer<_InputIterator>::__type _Integral;
>+      return __valid_range_aux(__first, __last, _Integral());
>     }
> 
>   template<typename _Iterator, typename _Sequence, typename _Category>