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.

Sign up for GitHub

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

Jump to bottom

Official support for views #301

Open
wants to merge 14 commits into
base: develop
Choose a base branch
from
Open

Official support for views #301

wants to merge 14 commits into from

Conversation

ldionne
Copy link
Member

@ldionne ldionne commented Sep 28, 2016 •
edited
Loading

The goal of this PR is to officially support views, which are only experimental right now.

Still left to do:

  • Move view types out of the experimental namespace, perhaps into detail
  • Consider providing access to the flattened methods & al. (not at first)
  • Test nested views
  • Add examples
  • Document views in the tutorial (requires some rewording of the sections on containers and on algorithms)
  • Provide operators (not at first)
  • Provide a non-dummy implementation of hana::equal (thanks @ricejasonf)
  • Provide a non-dummy implementation of hana::ap (thanks @ricejasonf)
  • Consider providing a filtered_view (not at first)

@ldionne ldionne self-assigned this Sep 28, 2016
@ricejasonf
Copy link
Collaborator

ricejasonf commented Jul 1, 2017 •
edited
Loading

I have a PR for equal, and I'd like to do ap as well. It appears that detail::cartesian_product_indices provides an interface that should make that easy to implement. A cartesian_product view looks pretty easy with that as well.

ldionne and others added 12 commits December 1, 2017 18:05
@ldionne
[view] Implement a flattened_view
fa92045
@ldionne
[view] Move out of the experimental namespace
22ac7ca
@ldionne
[view] Add a few automatic unit tests
fe5afcb
@ldionne
[view] Move view types out of the experimental namespace
33362d9
@ldionne
[view] Add tests for modifying the elements of a view
c6b41d7
@ldionne
[views] Take a reference in single_view to ensure proper semantics
b4b1554
@ldionne
[view] Add missing #include
2eaaa84
@ricejasonf @ldionne
[view] equal_impl
fad677f 
  - Improves equal_impl for view_tag using same implementation
    used for Sequences
@ricejasonf @ldionne
[view] cartesian_product
9050743 
  - Adds cartesian_product_view_t and cartesian_product_element_view_t
    with specializations for the following:
      - at_impl
      - is_empty_impl
      - length_impl
      - unpack_impl
@ldionne
[view] Make sure automatic unit tests use views for real
8b8a029
@ldionne
[view] Fix bug in drop_front
339366b
@ldionne
[view] Implement 'ap' using cartesian_product_view
58da968
@ldionne
Copy link
Member Author

ldionne commented Dec 2, 2017

I'm striking a few things that I marked as being required to ship this PR. Given my lack of time to work on Hana (very sadly), I'm going to go for a minimal implementation for the sake of shipping this. This PR is important and it has been sitting around for way too long.

@ricejasonf
Copy link
Collaborator

I didn't actually do ap yet, but it should be trivial now.

I could help with tests too if that is welcome.

@ldionne
Copy link
Member Author

ldionne commented Dec 2, 2017

I think the tests for ap are okay, what I'm mostly concerned about is tests for the rest of the functionality. For example, filter should work currently because view_tag implements MonadPlus, but I'm fairly confident it won't work properly (either compilation error or dangling reference) based on looking at how single_view is implemented.

So yeah, I'd welcome help on making the tests more robust. I'm happy to tackle the documentation side.

@ricejasonf @ldionne
[view] Use view_storage in views
5120ab4 
  - All views should use `view_storage` so there can't be
    dangling references to view objects when composing views.
  - Adds use of `view_storage` in `identity_view` and `single_view`
@ricejasonf
Copy link
Collaborator

ricejasonf commented Jan 3, 2018 •
edited
Loading

Are we going to make the views Sequence or is that some kind of violation?

(can't remember if I already asked this)

@ldionne
Copy link
Member Author

ldionne commented Jan 4, 2018 •
edited
Loading

They can't be Sequences because there's no way to make them from a pack of elements. In other words, they don't have storage like a Container, and Sequence requires that (actually Sequence might be better named Container, since it reflects element ownership).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@ldionne ldionne

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@ldionne @ricejasonf