Re: Pointlessly fragile constexpr vector implementation I need some guidance on.

Re: Pointlessly fragile constexpr vector implementation I need some guidance on.

[Jonathan Wakely]

On Wed, 28 Oct 2020 at 01:10, Josh Marshall via Gcc-help wrote:
>
> Hello all,
>
> This side project is definitely showing me a few of my shortcomings as a
> programmer, so please bear with me while I try to learn.  I have an
> implementation of constexpr vector which works in the null case, but fails
> on any case when it is constructed with contents or has contents added.

[Moved from gcc-help]

Comments below.

> diff --git a/include/ChangeLog b/include/ChangeLog
> index 41e3b76766e..d24b105b145 100644
> --- a/include/ChangeLog
> +++ b/include/ChangeLog
> @@ -22,6 +22,11 @@
>   * dwarf2.h (enum dwarf_sect_v5): A new enum section for the
>   sections in a DWARF 5 DWP file (DWP version 5).
>
> +2020-09-09  Caroline Tice  <cmtice@google.com>
> +
> + * dwarf2.h (enum dwarf_sect_v5): A new enum section for the
> + sections in a DWARF 5 DWP file (DWP version 5).
> +
>  2020-09-08  Felix Willgerodt  <felix.willgerodt@intel.com>
>
>   * floatformat.h (floatformat_bfloat16_big): New.

There are a couple of diffs in your branch that look like a bad merge,
they need to be removed. There's this one, and the change in
include/std/chrono which reverts a change I made in September.


> diff --git a/libstdc++-v3/include/bits/deque.tcc
> b/libstdc++-v3/include/bits/deque.tcc
> index 651ae70a84b..8236bef15e9 100644
> --- a/libstdc++-v3/include/bits/deque.tcc
> +++ b/libstdc++-v3/include/bits/deque.tcc
> @@ -974,6 +974,7 @@ _GLIBCXX_END_NAMESPACE_CONTAINER
>    // Overload for deque::iterators, exploiting the "segmented-iterator
>    // optimization".
>    template<typename _Tp, typename _VTp>
> +    _GLIBCXX20_CONSTEXPR
>      void
>      __fill_a1(const _GLIBCXX_STD_C::_Deque_iterator<_Tp, _Tp&, _Tp*>&

This function is unrelated to vector and should not be constexpr.

> __first,
>        const _GLIBCXX_STD_C::_Deque_iterator<_Tp, _Tp&, _Tp*>& __last,
> diff --git a/libstdc++-v3/include/bits/stl_algobase.h
> b/libstdc++-v3/include/bits/stl_algobase.h
> index d19f68871ab..128d8d8d4ad 100644
> --- a/libstdc++-v3/include/bits/stl_algobase.h
> +++ b/libstdc++-v3/include/bits/stl_algobase.h
> @@ -953,23 +953,24 @@ _GLIBCXX_END_NAMESPACE_CONTAINER
>      { std::__fill_a1(__first.base(), __last.base(), __value); }
>
>    template<typename _Tp, typename _VTp>
> +    _GLIBCXX20_CONSTEXPR
>      void
>      __fill_a1(const _GLIBCXX_STD_C::_Deque_iterator<_Tp, _Tp&, _Tp*>&,
>        const _GLIBCXX_STD_C::_Deque_iterator<_Tp, _Tp&, _Tp*>&,
>        const _VTp&);

This is the declaration of the function above, it shouldn't be constexpr.

>
>    void
> +  _GLIBCXX20_CONSTEXPR
>    __fill_a1(_GLIBCXX_STD_C::_Bit_iterator, _GLIBCXX_STD_C::_Bit_iterator,
>      const bool&);
>
>    template<typename _FIte, typename _Tp>
> -    _GLIBCXX20_CONSTEXPR
> -    inline void
> +    inline _GLIBCXX20_CONSTEXPR void
>      __fill_a(_FIte __first, _FIte __last, const _Tp& __value)
>      { std::__fill_a1(__first, __last, __value); }
>
>    template<typename _Ite, typename _Seq, typename _Cat, typename _Tp>
> -    void
> +    _GLIBCXX20_CONSTEXPR void
>      __fill_a(const ::__gnu_debug::_Safe_iterator<_Ite, _Seq, _Cat>&,
>       const ::__gnu_debug::_Safe_iterator<_Ite, _Seq, _Cat>&,
>       const _Tp&);

You've added constexpr here but not to the definition at
include/debug/safe_iterator.tcc:380 which gives errors.

I'm not sure if Debug Mode can be made compatible with constexpr. It
might be necessary to disable all Debug Mode checking for iterators
during constant evaluation. For now I would just forget about Debug
Mode, so don't add the CONSTEXPR macro to this overload.

> diff --git a/libstdc++-v3/include/bits/stl_vector.h
> b/libstdc++-v3/include/bits/stl_vector.h
> index d3f1b1fae5c..952f140fa0a 100644
> --- a/libstdc++-v3/include/bits/stl_vector.h
> +++ b/libstdc++-v3/include/bits/stl_vector.h
> @@ -1003,6 +1065,7 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
>         *  Returns true if the %vector is empty.  (Thus begin() would
>         *  equal end().)
>         */
> +      _GLIBCXX20_CONSTEXPR
>        _GLIBCXX_NODISCARD bool
>        empty() const _GLIBCXX_NOEXCEPT
>        { return begin() == end(); }

The constexpr macro needs to be after the [[nodiscard]] macro, or it
won't compile.



> diff --git
> a/libstdc++-v3/testsuite/23_containers/vector/constexpr/constructors.cc
> b/libstdc++-v3/testsuite/23_containers/vector/constexpr/constructors.cc
> new file mode 100644
> index 00000000000..27f458b9ec4
> --- /dev/null
> +++ b/libstdc++-v3/testsuite/23_containers/vector/constexpr/constructors.cc
> @@ -0,0 +1,139 @@
> +// { dg-do compile { target c++20 } }
> +// { dg-options "-std=c++20" }

Please use -std=gnu++20 not -std=c++20

> +
> +#include<cstddef>
> +#include<type_traits>
> +#include<vector>

Please use a space between the #include and header name.

> +
> +namespace constructor_group_one{
> +  constexpr std::vector<int> case_1;
> +  static_assert(case_1.size() == 0);
> +}
> +
> +
> +void constructor_group_two(){
> +  constexpr std::vector<int> case_2_1(0, 1);
> +
> +  static_assert(case_2_1.size() == 0);
> +
> +  //*

N.B. it's much easier to selectively comment out blocks using:

#if 0
...
#endif

You can just change 0 to 1 to enable the block.

> +  constexpr std::vector<int> case_2_2(1, 2);
> +  static_assert(case_2_2.size() == 1);
> +  static_assert(case_2_2.at(0) == 2);

This is a bug in your test, not your change to std::vector.

A non-empty vector can be used inside a constexpr function, it can't
be constructed as a constexpr variable. The "dynamic allocation" that
would be done inside a constexpr function is replaced by the compiler
with temporary storage during compilation, and must be freed again
within the same function. In other words, a constexpr vector can only
be a local variable inside a constexpr function.

You've attempted to create a constexpr variable at global scope, which
would mean that the temporary storage created during compilation would
have to remain available during run-time so that you can access the
vector members. That's impossible in C++20.

So you need to write your tests something like:

constexpr bool
constructors_group_two()
{
  std::vector<int> case_2_2(1,2);
  if (case_2_2.size() != 1)
    return false;
  if (case_2_2.at(0) != 2)
    return false;
  ...
  return true;
}

static_assert( constructors_group_two() );

This creates and manipulates a vector in a constexpr function, then
evaluates that function in a static assert, which forces the function
to be evaluated at compile-time.

You can also just use the usual VERIFY assertion macro that our tests
use for run-time tests:

#include <testsuite_hooks.h>

constexpr bool
constructors_group_two()
{
  std::vector<int> case_2_2(1,2);
  VERIFY( case_2_2.size() == 1 );
  VERIFY( case_2_2.at(0) == 2 );
  ...
  return true;
}

static_assert( constructors_group_two() );

This works because as long as the assertion succeeds, the VERIFY macro
doesn't do anything that isn't permitted in a constexpr function. If
the assertion fails, the macro will try to print an error and abort
the test, which isn't valid in a constexpr context, but that's fine,
it means the code will fail to compile at that point. Which is what
you want.

Tested correctly, your code seems to work (I only checked the
constructors and size() and at(size_type) members though).

Nice work so far!