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

Provide NonZero_c_* integers #82228

Merged
merged 3 commits into from
Feb 22, 2021
Merged

Provide NonZero_c_* integers #82228

merged 3 commits into from
Feb 22, 2021

Conversation

ijackson
Copy link
Contributor

@ijackson ijackson commented Feb 17, 2021

I'm pretty sure I am going want this for #73125 and it seems like an
omission that would be in any case good to remedy.

Because the raw C types are in std, not core, to achieve this we
must export the relevant macros from core so that std can use
them. That's done with a new num_internals perma-unstable feature.

The macros need to take more parameters for the module to get the
types from and feature attributes to use.

I have eyeballed the docs output for core, to check that my changes to
these macros have made no difference to the core docs output.

@rust-highfive
Copy link
Collaborator

r? @KodrAus

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 17, 2021
@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@ijackson ijackson marked this pull request as draft February 17, 2021 18:47
@thomcc
Copy link
Member

thomcc commented Feb 17, 2021

Given that c_int isn't a typesafe wrapper around e.g. i32, it seems reasonable to just have NonZero_c_int be a type alias for e.g. NonZeroI32. (substitute your platform's c_int size for i32)

That is, couldn't this just be a bunch of type aliases for the existing NonZero types added to std::os::raw?

(Regardless of your personal feelings about the choice for c_int to behave this way, it seems better to be consistent, no?)


Also, I personally think NonZeroCInt would be better a prettier color for the bikeshed than NonZero_c_int.

@ijackson
Copy link
Contributor Author

ijackson commented Feb 17, 2021 via email

This file contained a lot of repetitive code.  This was about to get
considerably worse, with introduction of a slew of new aliases.

No functional change.  I've eyeballed the docs and they don't seem to
have changed either.

Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>
I'm pretty sure I am going want this for rust-lang#73125 and it seems like an
omission that would be in any case good to remedy.

It's a shame we don't have competent token pasting and case mangling
for use in macro_rules!.

Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>
@ijackson
Copy link
Contributor Author

That is, couldn't this just be a bunch of type aliases for the existing NonZero types added to std::os::raw?

Thanks for the suggestion. I did this and it came out quite a lot nicer to implement too.

@ijackson ijackson marked this pull request as ready for review February 17, 2021 21:31
JohnTitor added a commit to JohnTitor/rust that referenced this pull request Feb 21, 2021
…k-Simulacrum

try-back-block-type test: Use TryFromSliceError for From test

Using `i32` is rather fragile because it has many implementations.  Recently in an early draft of another MR (rust-lang#82228) I did something that introduced a new `i32 as From<something>` impl and this test broke.

TryFromSliceError is nice because it doesn't seem likely to grow new conversions.  We still have one conversion, from Infallible.

My other MR is going to be reworked and won't need this any more but having done it I thought I would submit it rather than just throw it away.  Sorry for the tiny MR.
JohnTitor added a commit to JohnTitor/rust that referenced this pull request Feb 21, 2021
…k-Simulacrum

try-back-block-type test: Use TryFromSliceError for From test

Using `i32` is rather fragile because it has many implementations.  Recently in an early draft of another MR (rust-lang#82228) I did something that introduced a new `i32 as From<something>` impl and this test broke.

TryFromSliceError is nice because it doesn't seem likely to grow new conversions.  We still have one conversion, from Infallible.

My other MR is going to be reworked and won't need this any more but having done it I thought I would submit it rather than just throw it away.  Sorry for the tiny MR.
JohnTitor added a commit to JohnTitor/rust that referenced this pull request Feb 21, 2021
…k-Simulacrum

try-back-block-type test: Use TryFromSliceError for From test

Using `i32` is rather fragile because it has many implementations.  Recently in an early draft of another MR (rust-lang#82228) I did something that introduced a new `i32 as From<something>` impl and this test broke.

TryFromSliceError is nice because it doesn't seem likely to grow new conversions.  We still have one conversion, from Infallible.

My other MR is going to be reworked and won't need this any more but having done it I thought I would submit it rather than just throw it away.  Sorry for the tiny MR.
@KodrAus
Copy link
Contributor

KodrAus commented Feb 21, 2021

NonZeroCInt is definitely more palatable than NonZero_c_int, but I can appreciate the desire to use the latter.

This looks good to me! Thanks for the review @thomcc 🙇

@bors r+

@bors
Copy link
Contributor

bors commented Feb 21, 2021

📌 Commit 60a9dcc has been approved by KodrAus

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 21, 2021
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 22, 2021
Rollup of 9 pull requests

Successful merges:

 - rust-lang#82098 (Add internal `collect_into_array[_unchecked]` to remove duplicate code)
 - rust-lang#82228 (Provide NonZero_c_* integers)
 - rust-lang#82287 (Make "missing field" error message more natural)
 - rust-lang#82351 (Use the first paragraph, instead of cookie-cutter text, for rustdoc descriptions)
 - rust-lang#82353 (rustdoc: Remove unnecessary `Cell` around `param_env`)
 - rust-lang#82367 (remove redundant option/result wrapping of return values)
 - rust-lang#82372 (improve UnsafeCell docs)
 - rust-lang#82379 (Fix sizes of repr(C) enums on hexagon)
 - rust-lang#82382 (rustdoc: Remove `fake_def_ids` RefCell)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit a5f6668 into rust-lang:master Feb 22, 2021
@rustbot rustbot added this to the 1.52.0 milestone Feb 22, 2021
@ijackson ijackson deleted the nonzero-cint branch February 22, 2021 13:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants