Bug 4763 – std.stdio.File.open() : more efficient implementation

This is not a bug report, it's a list of three possible small changes to the code/API of std.stdio.File. Here Andrei says that the std.stdio.File.open() method is useful for performance: http://www.digitalmars.com/webnews/newsgroups.php?art_group=digitalmars.D&article_id=116278 Indeed inside File.this() there is an allocation, that open() may sometimes save if File.Impl.refs is 1. But open() contains: auto another = File(name, stdioOpenmode); That calls File.this(), so at a first sight it seems that open() performs the allocation anyway (if I am wrong then please ignore this). Generally it's positive to minimize APIs. You may write: auto f = File(...) f = File(...) Instead of: auto f = File(...) f.open(...) ---------------------- According to D specs on the site, inner structs contain an extra scope pointer, so this may be better: private static struct Impl { See also bug 4087 ---------------------- Given the presence of opAssign, I presume this code, inside open(): auto another = File(name, stdioOpenmode); swap(this, another); May be written just as: this = File(name, stdioOpenmode);

Cleanup: issue is very old (if any of the proposed changes is still needed, file seperately)

(In reply to Andre from comment #1) > Cleanup: issue is very old > > (if any of the proposed changes is still needed, file seperately) Reopening. Age is not a reason to close issues. A quick look suggests that the first point is still actual: `open` seems to do exactly the same as destructing and constructing [1]. Regarding the second point, as Steven Schveighoffer explains in issue 4087, comment 4, a struct declaration in another one does not make for context pointers. So that point is invalid. The suggested code change from the third point has apparently been done [1]. So that is fixed. So, what would be left to satisfy this enhancement request would be to deprecate and subsequently remove std.stdio.File.open. We could of course create a separate issue for that and close this one. I don't know if that buys us anything, though. [1] https://github.com/D-Programming-Language/phobos/blob/972227d286d2d31b76b32f3bc819e9aa50b8cfb0/std/stdio.d#L472-L476

I came to the same conclusion.. So we narrowed it down to one point, shall I change the title to reflect this? Regarding the follow up: Is someone in particular responsible for this library or the proces of deprecations for phobos? Do we need to test the deprecation, provide a rationale and corrective action? I'm willing to work on this as a way to learn the proces of contributing to D, but some guidance is helpful. Andre

(In reply to Andre from comment #3) > So we narrowed it down to one point, shall I change the title to reflect > this? Yeah, I suppose you can do that. > Regarding the follow up: > Is someone in particular responsible for this library or the proces of > deprecations for phobos? I don't think anyone is responsible for std.stdio or deprecations in particular. > Do we need to test the deprecation, provide a rationale and corrective > action? > > I'm willing to work on this as a way to learn the proces of contributing to > D, but some guidance is helpful. Are you familiar with git and GitHub? If not, you will have to learn the basics, at least. If you know your way around them, you can just clone the repositories and start making pull requests. See this page on the wiki: http://wiki.dlang.org/Starting_as_a_Contributor Regarding deprecations, I'm not sure what exactly the proper procedure is, i.e. what the phases are and how long they're supposed to be. Unfortunately, there doesn't seem to be any document on this. I would expect it to be on the wiki, but I couldn't find anything. You can ask about this in the forum, or you can just make a pull request with what feels ok to you and let reviewers correct you. Anyway, at some point you have to mark the thing `deprecated`, remove any references to it from the rest of the codebase, and finally (years later) remove the thing itself. Before all that happens, you have to get it through review, of course. And it's not clear yet if deprecating std.stdio.File.open is the way to go. Personally, I don't see a point in having `f.open(...)` when it does exactly the same as `f = File(...)`. But that's just me, and I only gave this a cursory look.

(In reply to ag0aep6g from comment #4) > (In reply to Andre from comment #3) > > Regarding the follow up: > > Is someone in particular responsible for this library or the proces of > > deprecations for phobos? > > I don't think anyone is responsible for std.stdio or deprecations in > particular. Jonathan Davis is the defacto deprecation manager. I think there should be an official document on how deprecations are to be handled, but if you ask on the forums, you will likely get an answer from him.

Performance of `open` is an implementation detail, but deprecation applies to interface. I'd say fix implementation to serve the intended purpose of the method. If it's impossible or undesirable then deprecate the method.

I did a code review, made some rough changes and tested it successfully. Conclusion: it's possible to implement a more efficient open() that re-uses the File struct state, avoiding the destructor/free + constructor/malloc pattern. I have to admit that the current implementation is shorter, easier to read. How much do we really need a more efficient File.open method? In what situations will this be beneficial?

(In reply to Andre from comment #7) > I have to admit that the current implementation is shorter, easier to read. > > How much do we really need a more efficient File.open method? In what > situations will this be beneficial? I don't know. Maybe make thread on the forum, or if the implementation is simple enough, just do it and see what reviewers say.

No allocation => win. I'm bootcamping this.

Commits pushed to master at https://github.com/dlang/phobos https://github.com/dlang/phobos/commit/c8751b245fcd19ef0f686bd82256d06d035b1c6d Fix Issue 4763 - std.stdio.File.open() : more efficient implementation added safe to setAppendWin https://github.com/dlang/phobos/commit/ca4677117f8cb408ee6ec4d09a5a614e4ac261da Merge pull request #5766 from jercaianu/open Fix Issue 4763 - std.stdio.File.open() : more efficient implementation merged-on-behalf-of: Andrei Alexandrescu <[email protected]>

Commits pushed to stable at https://github.com/dlang/phobos https://github.com/dlang/phobos/commit/c8751b245fcd19ef0f686bd82256d06d035b1c6d Fix Issue 4763 - std.stdio.File.open() : more efficient implementation https://github.com/dlang/phobos/commit/ca4677117f8cb408ee6ec4d09a5a614e4ac261da Merge pull request #5766 from jercaianu/open