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

IO: Fix OutputIteratorValueType being ignored #8155

Merged
merged 1 commit into from
Apr 25, 2024
Merged

IO: Fix OutputIteratorValueType being ignored #8155

merged 1 commit into from
Apr 25, 2024

Conversation

nh2
Copy link
Contributor

@nh2 nh2 commented Apr 21, 2024 •
edited
Loading

The template typename OutputIteratorValueType was unused.

This made specifying it (as suggested by the docs) ineffective, and forbade to use PLY functions e.g. with boost::function_output_iterator, or any other output iterator whose value_type is void.

Release Management

  • Affected package(s): Point_set_processing_3
  • License and copyright ownership: no change

@nh2
IO: Fix OutputIteratorValueType being ignored.
de41155 
The template typename `OutputIteratorValueType` was unused.

This made specifying it (as suggested by the docs) ineffective,
and forbade to use PLY functions e.g. with `boost::function_output_iterator`,
or any other output iterator whose `value_type` is `void`.
@lrineau lrineau added this to the 6.0-beta milestone Apr 22, 2024
@afabri
Copy link
Member

afabri commented Apr 22, 2024

It would be good to add Stream_support/test/Stream_support/issue8155.cpp which shows before/after. @soesau can you please do that.

@soesau soesau self-assigned this Apr 22, 2024
@sloriot
Copy link
Member

sloriot commented Apr 24, 2024

Successfully tested in CGAL-6.0-Ic-228

@lrineau lrineau added rm only: ready for master For the release team only: that indicates that a PR is about to be merged in 'master' TODO labels Apr 25, 2024
@lrineau
Copy link
Member

lrineau commented Apr 25, 2024

It would be good to add Stream_support/test/Stream_support/issue8155.cpp which shows before/after. @soesau can you please do that.

@soesau There is still that TODO.

@lrineau lrineau mentioned this pull request Apr 25, 2024
Closed
@lrineau lrineau removed the TODO label Apr 25, 2024
@lrineau
Copy link
Member

lrineau commented Apr 25, 2024

It would be good to add Stream_support/test/Stream_support/issue8155.cpp which shows before/after. @soesau can you please do that.

@soesau There is still that TODO.

I have created #8163 to track that TODO. I will merge this PR now.

@lrineau lrineau merged commit 1084b8b into CGAL:master Apr 25, 2024
8 checks passed
@lrineau lrineau removed the rm only: ready for master For the release team only: that indicates that a PR is about to be merged in 'master' label Apr 25, 2024
@lrineau
Copy link
Member

lrineau commented Apr 25, 2024

Now merged. Thanks @nh2 for your contribution.

@nh2
Copy link
Contributor Author

nh2 commented Apr 25, 2024

Thanks for the quick fix!

@soesau soesau mentioned this pull request Aug 28, 2024
Merged
sloriot added a commit that referenced this pull request Sep 3, 2024
@sloriot
added test for formerly unused OutputIteratorValueType (#8439)
761dab9 
## Summary of Changes

Follow-up of #8155. Added test.

The issue was only merged into master, but it also should have been
applied to cgal5.6

## Release Management

* Affected package(s): Point_set_processing_3 IO
* Issue(s) solved (if any): fix #8163
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lrineau lrineau lrineau approved these changes

Assignees

@soesau soesau

Labels
Enhancement Merged_in_6.0 Pkg::Stream_support Tested
Projects
None yet
Milestone
6.0-beta
Development

Successfully merging this pull request may close these issues.
5 participants
@nh2 @afabri @sloriot @lrineau @soesau