Std.stdio.file fixes #778

denis-sh · 2012-09-08T13:47:24Z

Refactor std.stdio.File
Fix Issue 7659 - std.stdio.File.close() erases file.name
Fix std.stdio.File part of Issue 4624

alexrp · 2012-09-08T13:53:36Z

392b3e76083bb8a4b18130d0d7f337c6a255cee6: LGTM.
8db9b54706bc5e91707eb6a849db4708aa58a78e: See comments.
c5c171a9e4126dfdca04223b93143f4185b5c434: LGTM.

alexrp · 2012-09-08T14:05:25Z

By the way, you may want to add yourself to the authors list.

denis-sh · 2012-09-08T14:33:20Z

"If" and 2x"that the" are fixed.
"" is now retured instead of null.

denis-sh · 2012-09-08T14:35:46Z

Why is this an assert? Did you mean to comment this out?

Can't find that comment... what line?

alexrp · 2012-09-08T14:37:04Z

Sorry, something went awry with that comment. I was asking why the assert in the name property is necessary.

denis-sh · 2012-09-08T14:48:17Z

Sorry, something went awry with that comment.

And it looks like it was 2 "if" comments but I have seen only one...

I was asking why the assert in the name property is necessary.

Just for tracking possible bugs in File implementation (e.g freed and overrited Impl).

P.S.

Damn Github doesn't show any of your old comments now and doesn't send line numbers in email notifications!

alexrp · 2012-09-08T14:50:22Z

And it looks like it was 2 "if" comments but I have seen only one...

Yeah, I thought the first one got lost, so I commented again. But it's OK, you've fixed all the issues I pointed out.

Just for tracking possible bugs in File implementation (e.g freed and overrited Impl).

So I presume that people using the name property don't have to fulfill that assert?

denis-sh · 2012-09-08T15:01:56Z

Just for tracking possible bugs in File implementation (e.g freed and overrited Impl).

So I presume that people using the name property don't have to fulfill that assert?

No they don't. The problem is in absence unified way to mark:

this tricks when our library is used incorrectly (asserss or Exceptions are used, but only asserss are acceptable IMHO);
this tricks when it's used correctly (only Exceptions are used, the most unified part);
this tricks when it is our bug in library (only asserss should be used, not sure where are examples).

alexrp · 2012-09-08T15:03:37Z

LGTM then.

denis-sh · 2012-09-08T15:28:36Z

LGTM then.

Sorry, one more commit. )

alexrp · 2012-09-08T15:29:45Z

Looks OK too.

denis-sh · 2012-09-11T20:55:49Z

Added 2 more commits to make std.stdio.File finally usable for non-Windows users (only non-Windows because of blocked by snn.lib Issue 7648 and nasty Issue 7033).

alexrp · 2012-09-12T00:15:19Z

5cf41e8d1e25ca639fd1376a941c906412b67bc5: Looks reasonable, but are you sure this won't break code?
d46d2e36159b38d5b08a91641d7e249bd9e6059b: LGTM.

denis-sh · 2012-09-12T07:24:43Z

5cf41e8d1e25ca639fd1376a941c906412b67bc5: Looks reasonable, but are you sure this won't break code?

It will. Just like any other incorrect behavior bugfix it will break the code relying on that. I just hope there is no (little) such code because personally I can't imagine me writing code that rely on undocumented fflush call in a function that never ever should call fflush.

andralex · 2012-09-17T02:58:25Z

Why is this failing?

denis-sh · 2012-09-17T12:39:42Z

Why is this failing?

You mean autotester, yes? As you see test dmd fails because runnable/statictor.d postscript sees empty output instead of this 8 lines...

Sorry, I have no idea (except another dmd bug) too. Will try to dig into.

denis-sh · 2012-09-17T13:54:52Z

OK. 5cf41e8d1e25ca639fd1376a941c906412b67bc5 breaks the code relying on write[f]ln flushing behavior. And test/d_do_test.d contains such code. Really sad. It passes on Windows because rawWrite flushes.

@braddr, is it a bug or do you really rely on this?

denis-sh · 2012-09-17T13:58:41Z

OK. It's clearly a bug. The test will fail once flush will be here at line 406 because postscript doesn't expect "Executing ..." message.

denis-sh · 2012-09-17T13:59:55Z

(I meant possibility of random failures)

denis-sh · 2012-09-17T14:46:08Z

Will try to fix this in dmd pull 1124.

denis-sh · 2012-09-17T17:17:36Z

Fixed. Have to wait for revising/merging dmd pull 1124.

andralex · 2012-09-28T11:58:32Z

std/stdio.d

    }
-    private Impl * p;
+    private Impl* p;
+    private string m_name;


we don't use the m_ convention anywhere. Either use name or use _name. In the latter case you'll need to also change p with _p.

Also, why migrating the name outside of Impl?

andralex · 2012-09-28T12:09:49Z

Some refactorings eliminating old bug workarounds are great (e.g. write*, various opens). I have a few nits with others as noted.

* also improve documentation a bit

denis-sh · 2012-10-04T06:56:50Z

To @andralex (rebased with the fallowing cosmetic fixes):

we don't use the m_ convention anywhere...

Renamed m_name to _name, p to _p.

not worth distinguishing between null and ""

Distinguishing removed. It was incorporated because of @alexrp comment:

I think this is a bit unfriendly, in terms of usability. Returning an empty string when no name is available seems better.

This leads to bad alignment. If there are two branches to an if, both should be either inline or on next line.

Alignment fixed.

denis-sh · 2012-10-04T07:03:42Z

Also, why migrating the name outside of Impl?

A copy of p.name in File is required to fix Issue 7659 and prepare to fixing part of Issue 4624.

denis-sh · 2012-10-04T07:27:59Z

This was intentional. Why remove it?

How interesting. Does it mean that it was intentional for std.stdio.File.writefln but not for std.stdio.File.writeln as I see no complains about the latter? Probably it doesn't and you are against entire Fix Issue 8219 - File.writeln is slow commit: 870bb84c21a20cab817cb69a54d26a16e553cb05.
My opinion is in commit description and in Comment 1 of Issue 8219.

And I'd like to see your explanation why it was added. Because it is undocumented, useless (see my opinion in commit description), inconsistent with IO in other languages, and even inconsistent with std.stdio.writeln. So it is obviously and completely unexpected performance and USB flash drive killer (now it is also SSD killer).

There is absolutely no need to explicitly flush a stream on new line because it is expected only for console output which is line buffered or unbuffered by default.

* don't throw AssertError in writef * make these functions consistent with std.format.formattedWrite

andralex · 2012-10-04T16:45:41Z

This was intentional. Why remove it?

How interesting. Does it mean that it was intentional for std.stdio.File.writefln but not for std.stdio.File.writeln as I see no complains about the latter? Probably it doesn't and you are against entire Fix Issue 8219 - File.writeln is slow commit: 870bb84.
My opinion is in commit description and in Comment 1 of Issue 8219.

And I'd like to see your explanation why it was added. Because it is undocumented, useless (see my opinion in commit description), inconsistent with IO in other languages, and even inconsistent with std.stdio.writeln. So it is obviously and completely unexpected performance and USB flash drive killer (now it is also SSD killer).

First of all, let's not forget (a) nobody is after you; (b) to the extent we're less competent than you think you are, it's not our fault; (c) we're all in this together as we want to do good things and move the state of the art forward.

The intent behind adding flush to writeln and writefln was simple - to make for more of a difference from write(..., '\n') and to behave unsurprisingly on block-buffered streams. So one would use writeln to get a buffered line, and write(..., '\n') to get an unbuffered one (on a block-buffered thing).

Anyhow, it seems this strategy is too obscure, so I'm fine with removing the flush. I'll make one more pass now through the latest. Keep in mind - we're on the same boat and we want to do Good Things(tm). Thanks.

Std.stdio.file fixes

andralex · 2012-10-04T16:51:20Z

merged

andralex reviewed Sep 28, 2012
View reviewed changes

denis-sh added 2 commits October 4, 2012 10:36

Refactor std.stdio.File

46a535c

Fix Issue 7659 - std.stdio.File.close() erases file.name

84a6468

* also improve documentation a bit

Fix std.stdio.File part of Issue 4624

26b7d3f

Remove redundant std.stdio.File.Impl.name

4c33328

denis-sh added 3 commits October 4, 2012 11:32

Rename std.stdio.File.p to _p

71fe49f

Fix Issue 8219 - File.writeln is slow

e281d8d

There is absolutely no need to explicitly flush a stream on new line because it is expected only for console output which is line buffered or unbuffered by default.

Improve std.stdio.File.{writef,writefln}

13a87ff

* don't throw AssertError in writef * make these functions consistent with std.format.formattedWrite

andralex added a commit that referenced this pull request Oct 4, 2012

Merge pull request #778 from denis-sh/std.stdio.File-fixes

ad87f38

Std.stdio.file fixes

andralex merged commit ad87f38 into dlang:master Oct 4, 2012

Uh oh!

Std.stdio.file fixes #778

Std.stdio.file fixes #778

Uh oh!

Conversation

denis-sh commented Sep 8, 2012

Uh oh!

alexrp commented Sep 8, 2012

Uh oh!

alexrp commented Sep 8, 2012

Uh oh!

denis-sh commented Sep 8, 2012

Uh oh!

denis-sh commented Sep 8, 2012

Uh oh!

alexrp commented Sep 8, 2012

Uh oh!

denis-sh commented Sep 8, 2012

P.S.

Uh oh!

alexrp commented Sep 8, 2012

Uh oh!

denis-sh commented Sep 8, 2012

Uh oh!

alexrp commented Sep 8, 2012

Uh oh!

denis-sh commented Sep 8, 2012

Uh oh!

alexrp commented Sep 8, 2012

Uh oh!

denis-sh commented Sep 11, 2012

Uh oh!

alexrp commented Sep 12, 2012

Uh oh!

denis-sh commented Sep 12, 2012

Uh oh!

andralex commented Sep 17, 2012

Uh oh!

denis-sh commented Sep 17, 2012

Uh oh!

denis-sh commented Sep 17, 2012

Uh oh!

denis-sh commented Sep 17, 2012

Uh oh!

denis-sh commented Sep 17, 2012

Uh oh!

denis-sh commented Sep 17, 2012

Uh oh!

denis-sh commented Sep 17, 2012

Uh oh!

andralex Sep 28, 2012

Choose a reason for hiding this comment

Uh oh!

andralex Sep 28, 2012

Choose a reason for hiding this comment

Uh oh!

andralex commented Sep 28, 2012

Uh oh!

denis-sh commented Oct 4, 2012

Uh oh!

denis-sh commented Oct 4, 2012

Uh oh!

denis-sh commented Oct 4, 2012

Uh oh!

andralex commented Oct 4, 2012

Uh oh!

andralex commented Oct 4, 2012

Uh oh!

Uh oh!