Properly close all open files on exit #121
Conversation
|
This looks good to me, and I certainly can't think of any bugs this could introduce. It's obviously hard to add a test for, but looks correct. Good catch! |
|
With regards the error, I personally would be tempted to instead add this as an "InfoWarning", at level 1. This means if anyone does have code which is relying on this behaviour (honestly, I'm not positive how they would be), it will at least still run. However, I imagine in most cases where people write such code it is an error. |
gap/io.gi
Outdated
| # so iterating over IO.OpenFiles directly would skip every | ||
| # second file. To avoid this, we iterate over a shallow copy | ||
| # of IO.OpenFiles. | ||
| for file in ShallowCopy( IO.OpenFiles ) do |
There was a problem hiding this comment.
Good catch!
Alternative implementation:
| for file in ShallowCopy( IO.OpenFiles ) do | |
| while Length(IO.OpenFiles) > 0 do | |
| file := Remove(IO.OpenFiles); |
This also avoids lots of unnecessary memory shuffling (though if you have so many files open that this would matter, you probably have other problems)...
There was a problem hiding this comment.
I think modifying IO.OpenFiles should only be done by the IO_... functions to ensure consistency, so I do not want to modify it by calling Remove(IO.OpenFiles);. I have pushed a different suggestion which avoids ShallowCopy, too.
The old logic iterated over a list while modifying this list, effectively skipping every second open file.
(How) could I use "InfoWarning" from C code? |
I guess one could import |
The old logic iterated over a list while modifying this list, effectively skipping every second open file.
To avoid such errors in my own code, I have added an error to my GAP which triggers if I remove elements from a list while iterating over it: zickgraf/gap@f8d4c35. Would this be a welcome change for GAP?