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.

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

Add std.stdio.File.setRawMode(). #3929

Closed
wants to merge 1 commit into from

Conversation

JohanEngelen
Copy link
Contributor

Especially for files that cannot be opened by the user in binary mode (like stdin and stdout), having only rawWrite and rawRead available is very limiting. To enable binary/raw file operation for stdin and stdout, the code to set those streams to binary mode is non-trivial. setRawMode() resolves this issue.

Especially for files that cannot be opened by the user in binary mode (like stdin and stdout), rawWrite and rawRead are very limiting. To enable binary/raw file operation for stdin and stdout, the code to set those streams to binary mode is non-trivial. setRawMode() resolves this issue.
@JohanEngelen
Copy link
Contributor Author

(made fixes, squashed into one commit again)

@Hackerpilot
Copy link
Member

Looks reasonable.

@quickfur
Copy link
Member

Hmm. I don't like exposing OS-level implementation details like this. I prefer lockingBinaryWriter as implemented in PR #2011. Under the hood it basically does the same thing, but the API is cleaner, works nicely as an output range, and fits in better with the already existing lockingTextWriter.

@CyberShadow
Copy link
Member

I don't disagree with the idea, but this needs to be a toggle. "Binary mode" is also the correct term here.

An alternative would be to provide a way to reopen stdin/out/err with arbitrary modes.

I don't like exposing OS-level implementation details like this.

For the record this is the C library API and Windows-only, so nothing OS-level.

BTW, this needs a correspoding Bugzilla issue.

@CyberShadow
Copy link
Member

https://issues.dlang.org/show_bug.cgi?id=9455
https://issues.dlang.org/show_bug.cgi?id=10545

Perhaps these can be combined into a property, binaryMode. This would allow querying and toggling either way.

@JohanEngelen
Copy link
Contributor Author

About lockingBinaryWriter: yes, would be great! But, it is not exactly the same: it locks.

About OS-level stuff: I think the basic problem is not setmode, it is the Digital Mars IO stuff that is troublesome. There should be a function (with sane name) for atomicOp!"&="(__fhnd_info[fd], ~FHND_TEXT);.

I first thought about setBinaryMode, setTextMode, but then I thought that that would somehow expose to much detail (and it doesn't actually set binary mode on non-Windows). Then I figured that the functionality is basically a permanent rawWrite, so "raw" stuck.
Anyway, I am happy to change the PR naming / toggling to your liking. However, I don't want to keep resubmitting this thing; so please discuss and come to a conclusion for me :-)

version(DIGITAL_MARS_STDIO)
{
import core.atomic;
immutable info = __fhnd_info[fd];
Copy link
Contributor Author

Choose a reason for hiding this comment

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

variable not used, remove.

@JakobOvrum
Copy link
Member

Note that if you include Fix ticket 7033 etc. in the commit message, a bot will automatically link them and mark them as resolved when/if the commits are merged.

$(LREF rawRead)
$(LREF rawWrite)
*/
void setRawMode()
Copy link
Member

Choose a reason for hiding this comment

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

The terminology is binary vs. text. Then we probably need a means to get the current mode, so there's no escaping the API:

enum FileMode { text, binary }
void getMode(FileMode);
void setMode(FileMode);

@DmitryOlshansky DmitryOlshansky added Needs Work @andralex Approval from Andrei is required labels Apr 11, 2016
@andralex
Copy link
Member

ping?

@JohanEngelen
Copy link
Contributor Author

sorry, guess I should have closed this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@andralex Approval from Andrei is required Needs Work
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants