Skip to content

Conversation

@alexrp
Copy link
Contributor

@alexrp alexrp commented Jun 10, 2012

This:

  • Documents stdin, stdout, stderr.
  • Marks some File members pure and nothrow.
  • Renames the previously undocumented isStreamingDevice to isFileDescriptor, deprecates the old name, and documents the new one properly.

Please merge before 2.060 if possible (and changes are OK).

requires: dlang/druntime#240

std/stdio.d Outdated
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FILE* and File aren't file descriptors. "File descriptor" is a POSIX term which refers to OS-level file handles, i.e. indices into a table of open files for a process. Quoting Wikipedia,

The FILE * file handle in the C standard I/O library routines is technically a pointer to a data structure managed by those library routines; one of those structures usually includes an actual low level file descriptor for the object in question on Unix-like systems. Since file handle refers to this additional layer, it is not interchangeable with file descriptor.

isFileHandle would be better, but I'm not sure if that is 100% appropriate either.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know it's not 100% accurate, but I think streaming device was a pretty bad term too... I'll rename it to isFileHandle though.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@jmdavis
Copy link
Member

jmdavis commented Jun 10, 2012

I thought that chunks and lines were functions. Everywhere else in Phobos that you construct a range like that, you do so by calling a function. I don't think that there are any other cases of constructing one via its constructor. So, this is already very bizarre. I do not like the idea of having to type

foreach(chunk; Chunks(stdin, 4096))

That looks just plain wrong and would break a lot of code. Now, if anyone is using chunks on the stack as a type, then I definitely think that Chunks makes more sense, but not in a foreach.

So, if we can reasonably rename chunks to Chunks while creating a function chunks which does the same thing that chunks' constructor does now without breaking any code, then I'm all for it (preferably making Chunks' constructor inaccessible in the process). If not, then I'm against this, and I would argue that the chunks to Chunks fix should be done with std.io when that is finally done. The same goes for lines and Lines.

@alexrp
Copy link
Contributor Author

alexrp commented Jun 10, 2012

I've added wrapper functions called lines and chunks that call the Lines and Chunks constructors, which are now private. I don't think this will break any code.

I have, however, added @disable this(); to those structs. This has higher potential for breakage, but I think it is a sensible change.

@DmitryOlshansky
Copy link
Member

Sounds sensible. I'm fine with it now.

@jmdavis
Copy link
Member

jmdavis commented Jun 10, 2012

You should change the foreach examples back to chunks and lines, and document the lines and chunks functions along with their structs (probably by putting them right next to each other and using /++ Ditto +/).

@jmdavis
Copy link
Member

jmdavis commented Jun 10, 2012

The other issue, can you get away with aliasing Chunks to chunks (and Lines to lines) to avoid breaking the cases where someone has explicitly used the type on the stack or as a member variable?

@alexrp
Copy link
Contributor Author

alexrp commented Jun 10, 2012

Done.

@alexrp
Copy link
Contributor Author

alexrp commented Jun 10, 2012

@jmdavis It could probably be done, but... I don't know. Then we're back to square one where we introduce random exceptions to our naming conventions. With my changes, you would have to do:

class A
{
    private Chunks c;
    this() { c = chunks(...); }
}

Which I don't think is that bad. We could of course open up the constructor so that c = Chunks(...); would be valid too. Basically my only gripe with the current situation is that a struct is named as if it was a function.

@jmdavis
Copy link
Member

jmdavis commented Jun 10, 2012

@alexrp I don't really see that as a problem. You have to do that with all of our other range types (it's particularly nasty with Voldemort types, though still doable). They're not really meant to be held as member variables like that, but people still do it from time to time, and in this case, they've been able to do it for ages, so even if we didn't really want them doing that, changing it now could break a fair bit of code. We don't really know how much, because we don't know how many people have been doing that.

The real question though is whether you can get away with making an alias of chunks for Chunks and doing

class A
{
    private chunks c;
    this() { c = chunks(...); }
}

until chunks as a type has been deprecated and removed. That is what would be needed to avoid breaking code.

And by the way, does making a member variable like that work with @disable this();, or does that just prevent explicit uses of Chunks.init? If it doesn't work, then we can't have @disable this();, even if we would have done it that way initially, because it would break code, and std.stdio is probably Phobos' most heavily used module.

@alexrp
Copy link
Contributor Author

alexrp commented Jun 10, 2012

It does work. The disable @this(); simply demands that c be initialized in all constructors.

But now I'm confused. Aliasing Chunks to chunks was exactly what I did initially....

@jmdavis
Copy link
Member

jmdavis commented Jun 10, 2012

Yes, it was what you did initially. The problem is that in the long term, we need chunks as a function, but in the short term, we we also need chunks as a type in order to avoid breaking code. So, if we can have the function and the alias, where the alias later gets deprecated and removed, then we're good. If not, then these changes may not be acceptable, because they will break code without providing a deprecation path.

As for disabling init, forcing that Chunks be initialized in the constructor will break code. It's perfectly possible at present to have one as a member variable which is not actually assigned until a particular function is called. As such, I don't think that we can disable it.

@alexrp
Copy link
Contributor Author

alexrp commented Jun 13, 2012

Yes, it was what you did initially. The problem is that in the long term, we need chunks as a function, but in the short term, we we also need chunks as a type in order to avoid breaking code. So, if we can have the function and the alias, where the alias later gets deprecated and removed, then we're good. If not, then these changes may not be acceptable, because they will break code without providing a deprecation path.

I don't think we can have both a function and a type alias. I don't see how the compiler would disambiguate. But on the other hand, I see no reasonable deprecation path at all, then. We need to do something...

As for disabling init, forcing that Chunks be initialized in the constructor will break code. It's perfectly possible at present to have one as a member variable which is not actually assigned until a particular function is called. As such, I don't think that we can disable it.

I'd argue that if people need an uninitialized Chunks, they should be using Nullable from std.typecons (keep in mind that accessing an uninitialized Chunks is always an error). But I don't have any strong feelings on the matter; I can revert the @disable this(); change if it's really too problematic in practice.

@jmdavis
Copy link
Member

jmdavis commented Jun 13, 2012

We might be able to get the compiler to disambiguate if the private constructor temporarily took a useless 3rd argument, then chunks(file, 4096) would clearly be the function rather than the constructor. But I don't know if that's enough to get the alias to work. It would have to be tried.

As for unitialized Chunks, I don't think that Nullable should generally be forced, so I'm not all that fond of the idea of that particular argument, but the problem here is that even if that's a really good argument, making that change will break code, which we really need to avoid - especially with something as heavily used as std.stdio.

If the alias doesn't work, then I'd be halfway tempted to just say screw it and risk the breakage on the theory that it should be abnormal to have the Chunks on the stack, but we're really trying to avoid breaking any code like that, and I'm sure that Walter will not like that (especially for std.stdio) and that he'd definitely argue to just leave it as-is rather than break things that way. That being the case, as desirable as this change may be, I'm not sure that we can make this change if it breaks code without going through the deprecation process first.

However, Steven has been working on std.io, and we could probably make the change there. Walter has insisted that its API be compatible with std.stdio, but given the way that chunks is typically used, the breakage of renaming it to Chunks and providing the free function chunks is minimal enough that we could probably get away with that change in std.io. So, I'd argue that if we can't get the alias to work alongside the free function that we should get the change made in std.io rather than in std.stdio. It delays the change, but both Walter and Andrei are increasingly adverse to breaking changes (or any changes that don't bring tangible benefit - meaning renaming stuff doesn't generally count), and Walter has indicated that he's particularly sensitive to std.stdio being broken, so much as I'd like to see std.stdio fixed, I'm very leery of making any breaking changes without providing a proper deprecation path.

@alexrp
Copy link
Contributor Author

alexrp commented Jun 13, 2012

If we can fix it in the long term in std.io, let us do that. This seems like a better solution.

@alexrp
Copy link
Contributor Author

alexrp commented Jun 13, 2012

Rebased. Please review again.

@jmdavis
Copy link
Member

jmdavis commented Jun 13, 2012

Okay. Looks good.

jmdavis added a commit that referenced this pull request Jun 13, 2012
@jmdavis jmdavis merged commit 646bb9e into dlang:master Jun 13, 2012
@jmdavis
Copy link
Member

jmdavis commented Jun 13, 2012

Merged.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants