Skip to content

Conversation

andralex
Copy link
Member

@andralex andralex commented Jun 8, 2013

I'm investigating semi-automated ways to minimize dependencies, and worked on std.algorithm by hand. I was unable to figure what's needed from std.range. For the others, I either pushed them down into unittests or made the symbols explicit.

One simple advice for Effective D would be:

"Avoid using version(unittest) import xxx;. Instead, use import xxx; inside every unittest that needs it."

@dnadlinger
Copy link
Contributor

I have a bad gut feeling about shipping this as long as http://d.puremagic.com/issues/show_bug.cgi?id=314 is still not RESOLVED FIXED.

@andralex
Copy link
Member Author

andralex commented Jun 8, 2013

@klickverbot good point. Could one of the compiler experts take a look at http://d.puremagic.com/issues/show_bug.cgi?id=314? @WalterBright? @9rnsr? @donc @AndrejMitrovic?

@monarchdodra
Copy link
Collaborator

Looking at the imports, I'd think a lot of these are probably only needed in specific functions. Using scoped imports would allow for safer (avoids bug 314), and more efficient (import only if used) imports.

In particular, I'm thinking about:

  • import std.container : BinaryHeap; : Probably only used by very specific functions?
  • import std.random : Random, uniform, unpredictableSeed; unittest only?

As for std.traits, we are using so much of it, I doubt a selective import is any useful.

@andralex
Copy link
Member Author

For a vanilla non-unittest import of std.algorithm, dependencies now are down from 55 to 31. To compute these numbers I ran:

../dmd/src/dmd -I../druntime/import  -w -d -m64  -O -release -c -o- std/algorithm.d -v | grep '^import ' | cat -n

@andralex
Copy link
Member Author

I can't further reduce dependencies because some symbols are used in restricted template guards, which cannot be scoped.

@ghost
Copy link

ghost commented Dec 20, 2013

We should avoid imports in version(unittest) blocks at all costs. The tests will always run with -unittest, which means we'll never catch undefined symbols referenced by templates when not using -unittest (i.e. in user code). We've already had a few bugs like this pop up.

@monarchdodra
Copy link
Collaborator

We should avoid imports in version(unittest) blocks at all costs.

In this case 'AFAIK), the only place where it is used is followed by a debug(std_algorithm) import std.stdio;, so I think it should be fine. unittests are not run with debug=std_algorithm.

@@ -1188,21 +1195,22 @@ Allows to directly use range operations on lines of a file.
}
}

import std.traits;
Copy link
Collaborator

Choose a reason for hiding this comment

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

That's a rather odd place to place a fully qualified import. This should go at the top I think. No one will go looking for an import in the middle of the file.

Copy link
Member Author

Choose a reason for hiding this comment

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

oops yes... shrapnel of various experiments.

@monarchdodra
Copy link
Collaborator

314 isn't fixed yet, is it? I like this, but I'm afraid that if we merge this right now, we'll leak symbols, leading to users writing code with incorrect imports...

For now, maybe we should leave the selective imports as global imports?

std.traits, std.typecons, std.typetuple, std.uni, std.utf;
import std.functional : unaryFun, binaryFun;
import std.range;
import std.traits;
Copy link
Collaborator

Choose a reason for hiding this comment

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

There's a couple extra localized import std.traits you added in the code, which are useless with this import right here.

@andralex
Copy link
Member Author

Presumably fixed Windows errors.

@andralex
Copy link
Member Author

passes tests, ping pliz pliz

WalterBright added a commit that referenced this pull request Dec 24, 2013
Minimized and explicit imports in std.algorithm
@WalterBright WalterBright merged commit 1a2152c into dlang:master Dec 24, 2013
@andralex andralex deleted the fewer-imports branch December 25, 2013 01:08
@9rnsr 9rnsr mentioned this pull request Dec 26, 2013
@9rnsr
Copy link
Contributor

9rnsr commented Dec 26, 2013

Unfortunately, this change silently breaks Phobos modules. Please see #1813 and merge it.

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.

5 participants