Skip to content

Don't use alias this in chunks implementation.#5689

Merged
andralex merged 2 commits intodlang:masterfrom
quickfur:chunks_fixes
Aug 11, 2017
Merged

Don't use alias this in chunks implementation.#5689
andralex merged 2 commits intodlang:masterfrom
quickfur:chunks_fixes

Conversation

@quickfur
Copy link
Member

Due to an oversight in PR #5624, an unnecessary alias this was left in the implementation of Chunks. This causes an import visibility error when Chunks is instantiated from another module. There is actually no need for this at all, since empty can just refer to impl directly.

I need some help with the unittest, though: the bug doesn't show up in a unittest in this module, obviously, because it only happens when Chunks is instantiated in another module. How to write a Phobos unittest in this case?? As in, where to put it?

@dlang-bot
Copy link
Contributor

Thanks for your pull request, @quickfur!

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.

@JackStouffer
Copy link
Contributor

unittest.d still exists for some reason; could use that

@quickfur
Copy link
Member Author

Unfortunately, that doesn't seem to be compiled by the makefile at all. :-(

@quickfur
Copy link
Member Author

LOL, apparently it's only built by the win64 and win32 makefiles.

Argh... hate this gratuitous makefile copy-pasta...

@wilzbach
Copy link
Contributor

Unfortunately, that doesn't seem to be compiled by the makefile at all. :-(

A file in std.internal then?

@quickfur
Copy link
Member Author

@wilzbach Probably a good idea. Which one, though?

@wilzbach
Copy link
Contributor

@wilzbach Probably a good idea. Which one, though?

A new one? DMD should be smart enough to avoid adding additional symbols when the entire module is empty and even if not, we can always use make to ignore / add it only to the test target.

@quickfur
Copy link
Member Author

Done, let's hope I didn't screw up the makefile too badly. :-D

@CyberShadow CyberShadow removed their request for review August 10, 2017 21:02
@PetarKirov
Copy link
Member

I think you need to update win32.mak and win64.mak, if I am not mistaken it should be the SRC_STD_INTERNAL rule.

One blunt way to ensure that the your changes to build system are ok is to make the test fail unconditionally and verify that it fails on all Autotester runners in about the same way.

@PetarKirov
Copy link
Member

Apart from the build system stuff, LGTM.

Needs to be a standalone module because the problem does not show up
within std.range itself.
@quickfur
Copy link
Member Author

Fixed windows makefiles.

Copy link
Member

@CyberShadow CyberShadow left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@andralex andralex left a comment

Choose a reason for hiding this comment

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

thx!

@andralex andralex merged commit 86144e6 into dlang:master Aug 11, 2017
@quickfur quickfur deleted the chunks_fixes branch August 11, 2017 18:31
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.

7 participants