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

[Jonathan Wakely <jwakely@redhat.com>]

On 30/09/19 22:21 +0200, François Dumont wrote:
>On 9/30/19 11:03 AM, Jonathan Wakely wrote:
>>These changes are fine but should have been a separate, unrelated
>>commit.
>
>Ok, sorry, I consider that just a comment change was fine.

It's fine, but it is unrelated so should be a separate commit. That
makes it easier to backport the documentation fix independently of the
rest of the patch. Or if the patch had to be reverted for some reason,
we wouldn't also revert the doc fix if it was in a separate commit.

Unrelated changes should usually be separate commits.


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

The standard says when something should be a constexpr function, we
don't get to decide. So if a function is not constexpr in C++17 and is
constexpr in C++20, we have to conform to that. For functions that are
not part of the standard and are our own implementation details, we
can choose when to make them constexpr. We could make all the debug
helpers use _GLIBCXX14_CONSTEXPR (or even _GLIBCXX_CONSTEXPR if they
meet the very restrictive C++11 constexpr requirements) but since they
are never going to be constant evaluated except in C++20, there
doesn't seem to be much point to doing that.