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

Close streams which are created in PrintCSV #3481

Merged
merged 1 commit into from
Jun 1, 2019
Merged

Close streams which are created in PrintCSV #3481

merged 1 commit into from
Jun 1, 2019

Conversation

ChrisJefferson
Copy link
Contributor

Code which calls PrintCSV many times with a filename will eventually start failing, as PrintCSV did not close files after it was finished with them.

@ChrisJefferson ChrisJefferson added kind: bug Issues describing general bugs, and PRs fixing them release notes: to be added PRs introducing changes that should be (but have not yet been) mentioned in the release notes labels May 31, 2019
@ChrisJefferson ChrisJefferson changed the title Close stream created in PrintCSV Close streams which are created in PrintCSV May 31, 2019
@coveralls
Copy link

coveralls commented May 31, 2019 •
edited
Loading

Coverage Status

Coverage decreased (-0.0002%) to 85.168% when pulling 6143a56 on ChrisJefferson:fix-printcsv into 5d81b55 on gap-system:master.

@fingolfin
Copy link
Member

It's really a shame that we don't have finalizers to take care of this!

fingolfin
fingolfin approved these changes May 31, 2019
View reviewed changes
tst/testinstall/stringobj.tst Show resolved Hide resolved
@codecov
Copy link

codecov bot commented May 31, 2019

Codecov Report

Merging #3481 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #3481      +/-   ##
==========================================
+ Coverage   85.34%   85.34%   +<.01%     
==========================================
  Files         699      699              
  Lines      346504   346509       +5     
==========================================
+ Hits       295731   295738       +7     
+ Misses      50773    50771       -2
Impacted Files Coverage Δ
lib/string.gi 86.37% <100%> (+0.07%) ⬆️
src/iostream.c 65.9% <0%> (+0.75%) ⬆️

@ChrisJefferson
Copy link
Contributor Author

I agree the lack of finalisers is annoying, in particular this is still "bugged" if we exit due to an error early, but in current GAP that's basically unfixable.

@fingolfin fingolfin merged commit 4e13271 into gap-system:master Jun 1, 2019
@ChrisJefferson ChrisJefferson deleted the fix-printcsv branch July 1, 2019 21:15
@wucas wucas added kind: bug: unexpected error Issues describing bugs in which computation unexpectedly encounters an error, and PRs fixing them release notes: added PRs introducing changes that have since been mentioned in the release notes and removed release notes: to be added PRs introducing changes that should be (but have not yet been) mentioned in the release notes labels Aug 20, 2019
@olexandr-konovalov olexandr-konovalov added this to the GAP 4.11.0 milestone Feb 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fingolfin fingolfin fingolfin approved these changes

Assignees
No one assigned
Labels
kind: bug: unexpected error Issues describing bugs in which computation unexpectedly encounters an error, and PRs fixing them kind: bug Issues describing general bugs, and PRs fixing them release notes: added PRs introducing changes that have since been mentioned in the release notes
Projects
None yet
Milestone
GAP 4.11.0
Development

Successfully merging this pull request may close these issues.
5 participants
@ChrisJefferson @coveralls @fingolfin @olexandr-konovalov @wucas