Skip to content

Conversation

@MetaLang
Copy link
Member

@MetaLang MetaLang commented Sep 6, 2017

No description provided.

@dlang-bot
Copy link
Contributor

Thanks for your pull request, @MetaLang! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.

Some tips to help speed things up:

  • smaller, focused PRs are easier to review than big ones

  • try not to mix up refactoring or style changes with bug fixes or feature enhancements

  • provide helpful commit messages explaining the rationale behind each change

Bear in mind that large or tricky changes may require multiple rounds of review and revision.

Please see CONTRIBUTING.md for more information.

Bugzilla references

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

std/stdio.d Outdated
}

// Undocumented but public because the std* handles are aliasing it.
@property ref File makeGlobal(int _iob)()
Copy link
Member

Choose a reason for hiding this comment

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

Would it be bad to make the enum an actual type, and then make the template parameter an instance of that enum? This would prevent for instance makeGlobal!5, which then also aliases to stderr without something like that. At the very least, this should say if(_iob >= stdin_handle && iob <= stderr_handle)

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I'll do that instead. My goal was to be as unintrusive as possible.

Copy link
Member

@schveiguy schveiguy left a comment

Choose a reason for hiding this comment

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

I'm not an expert on whether this will trigger -betterC problems, but this looks much better to me.

@MetaLang
Copy link
Member Author

MetaLang commented Sep 6, 2017

I believe we should be okay now that betterC uses C's assert.

@schveiguy
Copy link
Member

I'm just looking at the doc changes, I didn't realize the instantiation makes it into the docs. Something to think about for another PR I think.

@PetarKirov
Copy link
Member

PetarKirov commented Sep 7, 2017

I'm planning on nuking most of makeGlobal while addressing some of the thread-safety issues with std.stdio so I'm not sure if your efforts are worth it, though I haven't had the chance to work lately on this, so I'm not 100% sure how this will play with Walter's druntime PR, but I think I'll still use core.stdc.stdio.std* directly (I don't think I need to use them as template arguments).

@schveiguy
Copy link
Member

@ZombineDev while makeGlobal exists, we might as well make it work better than it does. Obviously Andrei thinks it's worthwhile, and the whole point is to enable support for using the libraries with -betterC. I am not sure exactly how this is going to work, or whether it will work. But if this is the push, we might as well not make it uglier than it has to be.

I'll warn you that std.stdio is a nest of hacks that is really difficult to untangle. I hope you can do it, but I definitely don't have much hope.

@MetaLang
Copy link
Member Author

MetaLang commented Sep 7, 2017

@ZombineDev I don't think it's a huge issue since this is such a simple PR. The code is working as-is so it doesn't matter if this gets merged or not; I just thought I'd fire a quick PR off based on Steven's comment here.

@PetarKirov
Copy link
Member

Alright, in that case feel free to proceed with this PR, as I can't give an ETA on my work, and as Steven points out we should strive to improve the status quo with what we have.

@dlang-bot dlang-bot merged commit 54cd27d into master Sep 7, 2017
@PetarKirov PetarKirov deleted the MetaLang-makeGlobal-enum branch September 7, 2017 22:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants