Removed version(unittest) blocks because they introduce big overhead

[alexandrumc]

user code:

module foo;

import core.sync.barrier;

unittest 
{
     assert(1 == 1);
}

library:

module core.sync.barrier;

version(unittest) 
{
      import core.sync.condition;
      import core.sync.semaphore;
      // other imports
      // classes
      // template instantiations
}

dmd -unittest foo.d

# will trigger semantic analysis on every top-level version(unittest) block from the library; 
# in the example above, this means semantic analysis also on condition, semaphore etc.
# -> makes the compilation take up to 5x longer on some examples

This is related to: dlang/phobos#6450

[dlang-bot]

Thanks for your pull request and interest in making D better, @alexandrumc! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

• My PR is fully covered with tests (you can see the coverage diff by visiting the details link of the codecov check)
• My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
• I have provided a detailed rationale explaining my changes
• New or modified functions have Ddoc comments (with Params: and Returns:)

Please see CONTRIBUTING.md for more information.

---

If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.

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.

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub fetch digger
dub run digger -- build "master + druntime#2888"

[Geod24]

Can you define what big overhead you are witnessing ?
The change in itself doesn't seem bad, but you probably want to make those functions static.

[alexandrumc]

@atilaneves

[alexandrumc]

Can you define what big overhead you are witnessing ?
The change in itself doesn't seem bad, but you probably want to make those functions static.

Please see the description I added. Thanks.

[wilzbach]

This doesn't fix the actual problem. The bug here is in DMD and this is just white-washing a tiny bit of the problem! There are likely many other occurrences of this in phobos or other libraries.

[atilaneves]

The bug here is in DMD and this is just white-washing a tiny bit of the problem!

Is version(unittest) iffy and does it have to be fixed? Yes. In the main problem in dmd? Yes. But:

• This PR makes things better wrt compilation times
• I think the code after the diff should have been what was there to begin with

There are likely many other occurrences of this in phobos or other libraries.

Unfortunately, yes. Also unfortunately, the fix in dmd is unlikely to be trivial (@alexandrumc has been looking into the root cause).

Even if version(unittest) didn't add overhead, however, I think this diff is still for the better.

[alexandrumc]

This doesn't fix the actual problem. The bug here is in DMD and this is just white-washing a tiny bit of the problem! There are likely many other occurrences of this in phobos or other libraries.

I know what the problem is. This is only a small part of the solution. The first step is to remove all the version(unittest) blocks, but I have to remove them in small incremental steps to be easier to review.

[Geod24]

@atilaneves : What's the plan with version (unittest) ? Asking because I hope it won't get the same treatment as unittest blocks, as they are two different beasts (namely: one can use version (unittest) to provide unittest-only behavior / utilities).

@alexandrumc : Thanks for adding the explanation.

[atilaneves]

@atilaneves : What's the plan with version (unittest) ? Asking because I hope it won't get the same treatment as unittest blocks, as they are two different beasts (namely: one can use version (unittest) to provide unittest-only behavior / utilities).

I'm not sure yet, I have to discuss it with Walter.

[schveiguy]

The issue with version(unittest) blocks specifically is that they generally are for testing the project or sometimes even only the module at hand. They are not usually for importers.

version(unittest) blocks get semantic analysis even if they are irrelevant to the importer. unittest functions do not. There has to be a very very good reason to have a version(unittest), whereas just including unittests is fine.

Thanks for the change, and hope to see more about the problem/solution described somewhere.

[CyberShadow]

Asking because I hope it won't get the same treatment as unittest blocks, as they are two different beasts (namely: one can use version (unittest) to provide unittest-only behavior / utilities).

How would that work? Wouldn't you need to include the module in the compilation to make use of said behavior/utilities, which would then enable version(unittest) (and unittest) blocks regardless?