Skip to content

Conversation

@alexandrumc
Copy link
Contributor

Related to dlang/druntime#2888

@dlang-bot
Copy link
Contributor

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 + phobos#7353"

@CyberShadow
Copy link
Member

Won't this break compiling user programs with -unittest? Meaning, due to -unittest, DMD will try to parse Phobos unittests too, which will now fail due to required definitions being versioned out?

@alexandrumc
Copy link
Contributor Author

alexandrumc commented Jan 15, 2020

Won't this break compiling user programs with -unittest? Meaning, due to -unittest, DMD will try to parse Phobos unittests too, which will now fail due to required definitions being versioned out?

No, because parsing doesn't have anything to do with valid definitions. Semantic analysis checks if the referred symbols are defined or not, but it is not done on the unittest {} blocks that are located in libraries.

@CyberShadow
Copy link
Member

No, because parsing doesn't have anything to do with valid definitions.

Sorry for using the wrong term :)

Semantic analysis checks if the referred symbols are defined or not, but it is not done on the unittest {} blocks placed in libraries.

Meaning, DMD will not look inside unittest blocks for files that are not on its command line, even with -unittest. I see.

Then, the question is, why not make version(unittest) behave the same way as unittest blocks? I.e. make the compiler ignore them if the file is not on its command line.

@alexandrumc
Copy link
Contributor Author

alexandrumc commented Jan 15, 2020

Then, the question is, why not make version(unittest) behave the same way as unittest blocks? I.e. make the compiler ignore them if the file is not on its command line.

Sure, a design decision must be made. However, we have to take into account the fact that version {} and unittest {} are different mechanisms, and ignoring version (unittest) might be only an unwanted particular case.

…essary overhead when compiling with -unittest
@alexandrumc alexandrumc force-pushed the replace-version-unittest branch from 30a20a5 to bb62aac Compare January 19, 2020 12:05
@alexandrumc
Copy link
Contributor Author

@atilaneves

endif

UDFLAGS=-unittest
UDFLAGS=-unittest -version=StdUnittest -version=CoreUnittest
Copy link
Contributor

Choose a reason for hiding this comment

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

Why include druntime's unittests with CoreUnittest?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Phobos uses https://github.com/dlang/druntime/blob/9baf9735c923a9bbb72b8e4ee05f513ea0499bbc/src/core/time.d#L120-L123 to run its unittests. But that version (unittest) will change into version (CoreUnittest) (dlang/druntime#2902), so the flag has to be there.

Copy link
Contributor

Choose a reason for hiding this comment

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

Right, -unittest cascades into druntime. Thanks for the explaination.

Copy link
Member

Choose a reason for hiding this comment

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

Should now be not necessary with my changes to dlang/druntime#2902.

Copy link
Member

Choose a reason for hiding this comment

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

Addressed in #7356.

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.

6 participants