Skip to content

libcore: Rename float constants for consistency. #16370

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

Closed
wants to merge 1 commit into from

Conversation

ruuda
Copy link
Contributor

@ruuda ruuda commented Aug 8, 2014

Rename f32::consts::PI_2 and f64::consts::PI_2 to TWO_PI for consistency with Float::two_pi. This deprecates PI_2 as suggested by @aturon. There are still functions in Float that have digits in their name, but I think it is hard to avoid that.

@aturon
Copy link
Member

aturon commented Aug 8, 2014

Thanks for doing this! It might also be worth changing constants like SQRT2 to be SQRT_2, LOG2 to LOG_2 etc.

@lilyball
Copy link
Contributor

lilyball commented Aug 8, 2014

I don't think it's appropriate to do this unless you rename all the other constants too. What are you going to rename FRAC_PI_2 to? Note that PI_2, etc are named after the M_PI_2, etc. constants from C's <math.h>.

@forticulous
Copy link

How about TAU?

@ruuda
Copy link
Contributor Author

ruuda commented Aug 9, 2014

For the fractions, I think PI_OVER_TWO would be more appropriate. They would have to be changed in Float as well. But what is the advantage of these fractions anyway? If you write Float::pi() / 2.0, the compiler should be able to create the constant at compile time, right? And pi() / 2.0 is shorter than frac_pi_2() (or one character longer if you need parentheses around it). I did a search on GitHub, and frac_pi_2 is only used in 16 places (excluding the definition, tests and wrappers). frac_pi_6() is not used anywhere except for the definition and tests, on GitHub at least. FRAC_2_PI is not used either.

For LOG2_E, the 2 there is part of the function name. I think the underscore separating the function and argument is fine if SQRT2 is renamed to SQRT_2.

Edit: Actually, having PI_OVER_TWO and SQRT_2 is not consistent either ... It could be HALF_PI, and FRAC_1_SQRT2 could be renamed to HALF_SQRT_2 (because sqrt(2)/2 = 1/sqrt(2)), but there would still be names with digits and names without. Any ideas?

@pnkfelix
Copy link
Member

pnkfelix commented Aug 9, 2014

@forticulous I doubt we'll see TAU; see discussion at PR #15248

(update: I worded that too strongly, especially given the number of core team members on #15248 who explicitly said that they did not object to tau. Still, it was a contentious PR.)

@ruuda ruuda changed the title libcore: Rename PI_2 to TWO_PI for consistency. libcore: Rename float constants for consistency. Aug 9, 2014
@aturon
Copy link
Member

aturon commented Aug 29, 2014

@ruud-v-a Sorry for the delay in getting back to you about this. I agree with @kballard that we need to consider the naming of these constants in a more global way, since the current names are meant to reflect the names in C. We're going through a "stabilization" process where we mark libstd APIs with stability attributes, and the float data types are one of the next targets.

If you'd like to help with this effort, you could start a thread on discuss with a broader proposal and rationale. Otherwise, the team will be dealing with floats soon and we'll take this inconsistency into account.

I'm going to close the PR for now, but thanks for bringing this to our attention!

@aturon aturon closed this Aug 29, 2014
@ruuda
Copy link
Contributor Author

ruuda commented Aug 29, 2014

I created a topic here.

@aturon
Copy link
Member

aturon commented Aug 29, 2014

@ruud-v-a Awesome, thanks so much for leading the charge on this!

@ruuda ruuda deleted the two-pi branch September 3, 2014 19:32
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 21, 2024
feat: Hover for literals showing additional value information
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.

5 participants