Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add size_type, difference_type to span #387

Closed
wants to merge 1 commit into from

Conversation

garyfurnish
Copy link
Contributor

Currently you can't use span generically as a replacement for std containers because it lacks size_type and difference_type, unlike std containers.

@rianquinn
Copy link
Contributor

Out of curiosity, could you give an example of what exactly this is fixing. Is the example something that could be added to the unit tests?

@garyfurnish
Copy link
Contributor Author

Sure. I was writing some super generic code with the goal of using almost always auto together with types to make a function that performs actions on segments of a container (such as span, various flat sets, vector, etc). Works fine on standard libraries, but there is no container traits to get size_type or difference_type for looping (and since its a partition function, you need actual integral types, not just an iterated loop).
(I just wrote this, so I do not claim it has no bugs).

  /**                                                                                                                                    
   * \brief Partition a container over some other container using a lambda.                                                              
   *                                                                                                                                     
   * On every element of over container, call F(over container element, make_tuple(begin,end)).                                          
   * Where begin and end are iterators over a segment of the partitioned container                                                       
   * such that the segments are as close to equal length as possible.                                                                    
   * @param pc Container to partition                                                                                                    
   * @param oc Container to partition over.                                                                                              
   * @param f Function to call on over container elements and segments.                                                                  
   **/
  template <typename PartitionContainer, typename OverContainer, typename F>
  void equipartition(PartitionContainer &&pc, OverContainer &&oc, F &&f)
  {
    if (oc.empty() || pc.empty())
      return;
    using over_container_type = typename ::std::decay_t<OverContainer>;
    const auto num_over = ::gsl::narrow<typename over_container_type::difference_type>(oc.size());
    const auto num_partition = ::gsl::narrow<typename ::std::decay_t<PartitionContainer>::difference_type>(pc.size());
    const auto data_per_partition = num_partition / num_over;
    for (::std::remove_cv_t<decltype(num_over)> i = 0; i < num_over - 1; ++i)
      f(oc[static_cast<typename over_container_type::size_type>(i)],
        ::std::make_tuple(pc.begin() + data_per_partition * i, pc.begin() + data_per_partition * (i + 1)));
    f(oc.back(), ::std::make_tuple(pc.begin() + data_per_partition * (num_over - 1), pc.end()));
  }

I'm not exactly sure how to turn this into a test given it was a compile error.

@garyfurnish
Copy link
Contributor Author

garyfurnish commented Oct 6, 2016

After checking, string_view and the proposed array_view (not sure if that made a TS or not, http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2015/p0122r0.pdf) and the proposed span (http://open-std.org/JTC1/SC22/WG21/docs/papers/2016/p0122r3.pdf) aren't technically containers, and neither is gsl::span because they don't control their own memory. This seems awful given the proliferation of container like things in the future standard. However, both string_view, array_view, and span implement the full container concept in their standards documents even though they are not actually containers. I'd say the trend seems to be that "container-like" things/views should follow the container concept when possible. (Really it seems like there needs to be some view concepts).

I could use size() to get the size_type, and then use iterator and iterator traits to get difference_type. This is ugly though and still really contains an implicit reliance on the container concept giving iterator; you really can't get away from it sneaking in.

@garyfurnish
Copy link
Contributor Author

garyfurnish commented Oct 6, 2016

Actually I'm wrong, proposed span does not have size_type. It does have difference type. Maybe this should be changed?

@gdr-at-ms
Copy link
Member

General note: span<T> and other view abstractions aren't containers and we shouldn't try hard to make them look like one. If you are writing a generic code and you are assuming containers, but actually using views, you are doing something wrong.

@garyfurnish
Copy link
Contributor Author

I don't know that I agree with that. If you can have a function that works with a const vector, it should probably work out of the box with a span. There is at least one major bug here: difference_type is specifically included in P0122R3 for precisely this reason and this repository is supposed to be a reference implementation of that according to the paper. size_type isn't mentioned. I'm assuming oversight, but maybe there is a reason? Maybe @neilmacintosh can weigh in?

@neilmacintosh
Copy link
Collaborator

@garyfurnish Sorry to let this thread run so long, but I was busy elsewhere today.

span has no size_type by-design (you observe correctly that it is not present in the standardization paper), so I'm going to have to reject this PR.

In the current standard library, size_type is only offered by container types and it is always unsigned. span is not a container type, nor should it pretend to be one. span consciously chooses to index using a signed type (the reasons for this have been discussed here before) and so to use a signed size_type would promote confusion.

Both of these positions met with approval from the Library Evolution Working Group in standardization meetings (in fact, not having size_type was specifically requested by a number of members of that committee).

Hope that helps to clear things up.

@garyfurnish
Copy link
Contributor Author

@neilmacintosh That actually really clarifies things on size_type (and makes sense too). What about difference_type?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants