Skip to content
This repository was archived by the owner on Oct 12, 2022. It is now read-only.

Move emplaceRef to core/internal #2498

Merged
merged 1 commit into from
Mar 1, 2019
Merged

Conversation

edi33416
Copy link
Contributor

Move emplaceRef to core.internal.lifetime so we can remove the code duplication from Phobos.
emplaceRef is intentionally non-ddoc'ed

@dlang-bot
Copy link
Contributor

Thanks for your pull request and interest in making D better, @edi33416! 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 annotated coverage diff directly on GitHub with CodeCov's browser extension
  • 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#2498"

@thewilsonator
Copy link
Contributor

Why does this need to be moved for phobos deduplication?

@thewilsonator
Copy link
Contributor

At the very least it should be imported by core.lifetime

cc'ing @TurkeyMan

@edi33416
Copy link
Contributor Author

Why does this need to be moved for phobos deduplication?

emplaceRef is used internally in various parts of Phobos. See @wilzbach 's comment here

Copy link
Member

@PetarKirov PetarKirov left a comment

Choose a reason for hiding this comment

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

Moving to core.internal is good as it's the place for public, yet intended for internal use, functions.
A "simpler" way would be to make emplaceRef public and leave it in core.lifetime, but I don't think we should advertise emplaceRef as part of the public API, or at least not yet.

Copy link
Member

@wilzbach wilzbach left a comment

Choose a reason for hiding this comment

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

Thanks!

@wilzbach wilzbach merged commit d10f515 into dlang:master Mar 1, 2019
@wilzbach
Copy link
Member

wilzbach commented Mar 1, 2019

I don't think we should advertise emplaceRef as part of the public API, or at least not yet

Imho it's not going to go anywhere anyhow (like emplace), so we could just start to officially lock us in ;-)
Though, of course core.internal is safer and solves the duplication problem.

@edi33416
Copy link
Contributor Author

edi33416 commented Mar 1, 2019

Why does this need to be moved for phobos deduplication?

At the very least it should be imported by core.lifetime

cc'ing @TurkeyMan

I chose local imports against top level ones so a user won't pay the import cost if he doesn't use emplace.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants