Array review comments

From: Mark Rodgers (mark.rodgers_at_[hidden])
Date: 2000-06-27 06:13:49

I vote for inclusion in Boost.  However what do we do now that
we have two array.hpp's?  Is Dietmar's to be renamed?

Below are my comments.  Some points have already been noted by 
others but I repeat them to show that I agree with them.

Design

- I think the design is sound and that it adheres as closely
  to the sequence requirements as is possible given the constraints.

- The ability to support aggregate initialisation is very important
  and should certainly be preserved.  I do not believe that the 
  indeterminate nature of arrays of built-in types is a concern since
  they can easily be all default initialised with an empty 
  initialiser list.

- I would like to see some support for assignment.  Maybe some or 
  all of the following additional members:

     template <class U> array &operator=(const array<U,N> &a);
     template <class U> void assign(const array<U,N> &a);
     template <class II> void assign(II first, II last);
     void assign(const T &u, size_type n = N);

- I agree that size(), empty() and max_size() should be static.

- I think rangecheck should be private.  Note that this does not 
  prevent it being an aggregate - the aggregate restriction is no
  private or protected non-static data members.

- rangecheck should definitely be a const function.

- I agree that an enum for N should be provided, and that 
  static_size is probably the best name yet.

- I'm not sure if swap should be provided.  Using std::swap,
  which would use the compiler generated assignment operator, 
  could well be faster in certain circumstances (but at the
  expense of possibly a large amount of stack space).  A quick
  test swapping two arrays of 1000 integers showed 
  std::swap(a1,a2) took only 60% of the time taken by a1.swap(a2).

- I don't think the arguments to the global swap are supposed to be
  const!

Implementation

- The size_t/ptrdiff_t hack should be removed in favour of
  including boost/config.hpp.

- empty() should just return false.  N can never be zero.

- swap (if it is to be provided) should use std::swap_ranges.

- operator== should use std::equal.

- operator< should use std::lexicographical_compare.

- rangecheck should provide a more helpful message in the exception.

Documentation

- I'd prefer the documentation to be Boost-ified.

- "This results in some overhead in case only arrays with static size 
  are needed."  Perhaps this should be "This results in some overhead in 
  situations where arrays with static size are all that is needed."

- "many feedback" should be "a lot of feedback".

- Instead of, "It doesn't fulfill the requirements of a sequence, except 
  that" perhaps it could be "It doesn't fulfil any of the additional 
  requirements for a sequence, except that".

- Open issues should be removed because these should be resolved by the
  time of final submission:

  1. Yes, I do want initialiser list support.
  2. Yes, I believe the extra braces can be elided (11.5.1p11).
  3. I don't see anything wrong with array4.cpp.  A Borland bug prevents
     it working with that compiler, but it seems to be fine with gcc.
  4. I haven't seen Jeremy's mail.  Why should mismatch be faster?  I
     find it hard to see how it could be.
  5. What does "static_casts for reverse iterator stuff" mean?

Tests

- #include <boost/array.hpp> or #include "boost/array.hpp"  throughout.

- Why all the loops to print elements instead of simply doing a std::copy 
  to an ostream_iterator?

- array3 and array4 don't work with Borland compilers - they are not 
  happy to convert the string literals in the initialiser lists to 
  the std::strings in the arrays.  Using array<const char*, ...> 
  instead tends to make it happier.

- array4 probably works better if the inner loop is

        for (unsigned j=0; j<seasons.size(); ++j) {
            std::cout << " *" << seasons[j] << "*";
        }

Mark