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

Fix vertex istream operator #3172

Merged
merged 14 commits into from Nov 12, 2018

Conversation

sloriot
Copy link
Member

@sloriot sloriot commented Jun 15, 2018

Reported on cgal-discuss.

@sloriot
Copy link
Member Author

sloriot commented Jun 15, 2018

@cjamin @mglisse I updated the testsuite to test the IO, but I have a runtime error. Did I miss something?

@lrineau lrineau added the Not yet approved The feature or pull-request has not yet been approved. label Jun 15, 2018
@mglisse
Copy link
Member

mglisse commented Jun 15, 2018

operator>> for points seems broken. It is in a wrong file, and expects to read something different from what operator<< prints.

@lrineau
Copy link
Member

lrineau commented Jun 21, 2018

@mglisse Where is the implementation of operator>>. Could you please fix it?

@mglisse
Copy link
Member

mglisse commented Jun 21, 2018

Where is the implementation of operator>>. Could you please fix it?

It is in a weird place (Triangulation/IO but it should be in NewKernel_d if it exists at all). No time right now, I am traveling.

@lrineau
Copy link
Member

lrineau commented Jul 10, 2018

This PR is the only thing that "blocks" CGAL-4.11.2. I will postpone it to CGAL-4.12.1, or later, so that I can release CGAL-4.11.2 and close that release branch.

@lrineau lrineau modified the milestones: 4.11.2, 4.12.1 Jul 10, 2018
@lrineau lrineau closed this Jul 10, 2018
@sloriot sloriot changed the base branch from releases/CGAL-4.11-branch to releases/CGAL-4.12-branch July 10, 2018 15:48
@sloriot sloriot reopened this Jul 10, 2018
@lrineau lrineau removed this from the 4.12.1 milestone Sep 4, 2018
@lrineau lrineau added this to the 4.14-beta milestone Oct 1, 2018
@lrineau
Copy link
Member

lrineau commented Oct 1, 2018

@mglisse commented on Jun 21, 2018, 3:00 PM GMT+2:

Where is the implementation of operator>>. Could you please fix it?

It is in a weird place (Triangulation/IO but it should be in NewKernel_d if it exists at all). No time right now, I am traveling.

Marc, could you comment, now?

@mglisse
Copy link
Member

mglisse commented Oct 3, 2018

Branch mglisse/NewKernel_d-istream-glisse ?
For a 3D point with coordinates 7, 8, 9, we output 3 7 8 9. The code in Triangulation was expecting to read just 7 8 9, I changed it to expect 3 7 8 9. For fixed dimension, we could use 7 8 9, but for dynamic dimension it was forcing Clement's code to read a whole line and expect that it contains exactly the whole point and nothing else.

@lrineau
Copy link
Member

lrineau commented Oct 4, 2018

@sloriot @mglisse I let you discuss about the best way to fix the issue...

@sloriot
Copy link
Member Author

sloriot commented Oct 4, 2018

@mglisse I cherry-picked your commits in this branch.
I have to do 2 more modifs so that it works. @cjamin could you check 4ed546e and 3ed89cb?

@lrineau
Copy link
Member

lrineau commented Oct 8, 2018

There is an issue with the base branch of this PR:

sloriot wants to merge 1,920 commits into CGAL:releases/CGAL-4.12-branch

@lrineau lrineau added Not yet approved The feature or pull-request has not yet been approved. and removed Not yet approved The feature or pull-request has not yet been approved. Under discussion labels Oct 8, 2018
@sloriot
Copy link
Member Author

sloriot commented Oct 11, 2018

@maxGimeno merged master some some reason. I removed that commit and we're back to 11 new commits.

@maxGimeno
Copy link
Contributor

I merged master because I couldn't use the cgal root as a cgal dir. I guess the branch is older than the working header only mechanism, but I didn't think of it at the time.

@lrineau
Copy link
Member

lrineau commented Oct 12, 2018

Let me know once the PR is ready to be integrated.

@cjamin
Copy link
Member

cjamin commented Nov 6, 2018

Hi. Sorry I missed this discussion, but to be honest, I hardly remember anything about this, except the code was a bit messy and that I didn't spend much time on trying to clean it up. Thanks for the fixes.

@sloriot sloriot removed the Not yet approved The feature or pull-request has not yet been approved. label Nov 8, 2018
@sloriot
Copy link
Member Author

sloriot commented Nov 12, 2018

Successfully tested in 4.14-Ic-40.

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

Successfully merging this pull request may close these issues.

None yet

6 participants