Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix issue 16634 - std.math exposes yl2x and yl2xp1 publicly #4975

Merged
merged 1 commit into from Dec 22, 2016

Conversation

somzzz
Copy link
Contributor

@somzzz somzzz commented Dec 20, 2016

There was duplicate code in core.math and std.math: the same function declarations and the same unittests. yl2x and yl2xp1 are implemented in the compiler.

There is indeed no proper documentation for their existence in phobos. I could not find documentation for core.math.yl2xp1 either. Maybe it should be added as part of this PR?

So far, I removed these functions from phobos, since they are available in core.math and std offers no extra functionality with regard to them. Please let me know if there is a valid reason for keeping them in both druntime and phobos. If that is the case, we can simply document the situation.

@dlang-bot
Copy link
Contributor

Fix Bugzilla Description
16634 std.math exposes yl2x and yl2xp1 publicly

@wilzbach wilzbach added the math label Dec 20, 2016
@wilzbach
Copy link
Member

So far, I removed these functions from phobos, since they are available in core.math and std offers no extra functionality with regard to them. Please let me know if there is a valid reason for keeping them in both druntime and phobos. If that is the case, we can simply document the situation.

Git log says that core.math was created by @braddr and yl2x / yl2xp1 in Phobos come from @WalterBright.
What's the history here?

There is indeed no proper documentation for their existence in phobos. I could not find documentation for core.math.yl2xp1 either.

There is only a mere one-line :/

https://dlang.org/phobos/core_math.html#.yl2x

@andralex
Copy link
Member

@WalterBright @9il @ibuclaw could you please take a look at this?

@9il
Copy link
Member

9il commented Dec 21, 2016

No math changes, the only question is proper intrinsics location. Please wait review from DRuntime/Compiler team members.

@UplinkCoder
Copy link
Member

The intrinsic is core.math.{yl2x, yl2xp1}.
phobos functions may not be intrinsics.
The PR looks fine to me.

@andralex
Copy link
Member

Thanks everyone!

@andralex
Copy link
Member

Auto-merge toggled on

@andralex andralex merged commit 6acf4a2 into dlang:master Dec 22, 2016
@WalterWaldron
Copy link
Contributor

Shouldn't this have gone through a deprecation cycle?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
7 participants