Skip to content

Start using the ddmd frontend to more narrowly scope imports #2809

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

Merged
1 commit merged into from
Jan 1, 2015
Merged

Start using the ddmd frontend to more narrowly scope imports #2809

1 commit merged into from
Jan 1, 2015

Conversation

joakim-noah
Copy link
Contributor

I just modified the ddmd frontend to dump all imported symbols, based on Daniel's newmagic ddmd branch from a couple weeks back. I was able to make these small cleanups by running this modified ddmd on std.stdio and looking for places to tighten up the imports, doing more soon.

I'm posting this PR early so that it can be looked over as I go, and run through the auto-tester.

@joakim-noah
Copy link
Contributor Author

I think I'm done with std.stdio, more narrowly scoping as many imports as I could, all from druntime. Given issue 314, ie module-scope selective imports are incorrectly made public, I did not use any selective imports at module scope, even reversing the recent selective import of Flag so that it isn't made public. I'd like to have reversed the selective import of HANDLE on Windows also, but that made it into official releases, so I figured better not break code that relies on that bug, at least until issue 314 is fixed in dmd.

Since I can't make them selective yet, I documented the symbols imported by the remaining module-scope imports in comments. I was able to remove some static imports after more narrowly scoping core.sys.posix.stdio, since functions like fileno, fopen, and fdopen are confusingly declared twice, there and in core.stdc.stdio.

This PR is now ready for review. If these kinds of changes are found worthwhile, I may submit more PRs for other modules.

@joakim-noah joakim-noah changed the title WIP: Start using the ddmd frontend to more narrowly scope imports Start using the ddmd frontend to more narrowly scope imports Dec 26, 2014
@joakim-noah
Copy link
Contributor Author

Updated this pull to localize symbols where possible from module-scope imports std.range.primitives and std.traits, so that the remaining commented-out module-scope symbols can be made into a selective import once issue 314 is fixed.

Should be finally done now.

@DmitryOlshansky
Copy link
Member

Looks nice but I worried if some symbols are only required on some platforms and not the other.
std.stdio is not that well covered by tests to be certain...

@joakim-noah
Copy link
Contributor Author

The auto-tester did catch some of these for me on FreeBSD or Windows that ddmd didn't, since I was only running the ddmd frontend for linux. If the test coverage isn't complete, it's possible that some other missed dependency slipped through.

But maybe that's a reason to do it anyway, to help us find where the test coverage needs to be improved?

@joakim-noah
Copy link
Contributor Author

Oh, also, many phobos modules were recently updated by @9il (#2727 for this module) to more narrowly scope other phobos imports, so if they're going to break because of bad test coverage, that's already been done.

@@ -17,13 +17,12 @@ Authors: $(WEB digitalmars.com, Walter Bright),
module std.stdio;

public import core.stdc.stdio;
static import core.stdc.stdio;
import std.typecons : Flag;
Copy link

Choose a reason for hiding this comment

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

Why was a selective import replaced with a regular import? Seems like it's doing the opposite of what we need.

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 laid out the reason in my second comment above. Issue 314 is that selective imports at module scope are currently always public, ie those symbols are leaked into any module that imports this module. Since this selective import was added between major releases, I thought it best to reverse it, until issue 314 is fixed, so that it won't leak in the next release.

Copy link

Choose a reason for hiding this comment

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

Ah right, forgot about that. Cool. :)

@ghost
Copy link

ghost commented Dec 31, 2014

std.stdio is not that well covered by tests to be certain...

Yeah I think we need to have some kind of version (TestStdio) stdout = <some_output_buffer> or something to that effect, and then write a proper test-suite for std.stdio. Just relying on intuition that it works is not good.

But that's another step for later. This mostly LGTM. Will have another deeper look later and merge.

@ghost ghost self-assigned this Dec 31, 2014
@ghost
Copy link

ghost commented Jan 1, 2015

Auto-merge toggled on

ghost pushed a commit that referenced this pull request Jan 1, 2015
Start using the ddmd frontend to more narrowly scope imports
@ghost ghost merged commit 8e4df7f into dlang:master Jan 1, 2015
@joakim-noah joakim-noah deleted the imports branch February 22, 2015 18:36
@ghost ghost removed their assignment Apr 28, 2015
This pull request was closed.
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.

2 participants