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

Removed ascii_ctype #48430

Closed
wants to merge 3 commits into from
Closed

Removed ascii_ctype #48430

wants to merge 3 commits into from

Conversation

hellow554
Copy link
Contributor

This is stable since rust 1.24.0 and is no longer behind a feature gate

@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @aturon (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 22, 2018
@mattico
Copy link
Contributor

mattico commented Feb 22, 2018

Reference: #39658 (comment)

Functions were initially implemented on AsciiExt, then moved to inherent impls on u8 and char, then stabilized.

It's probably time to remove the old AsciiExt methods (here), but that doesn't have to happen in this PR.

@hellow554
Copy link
Contributor Author

Didn't see that issue. I will adresse the other tasks as well (I hope), give me one day :)

@hellow554
Copy link
Contributor Author

@mattico I may need your help here.

error[E0407]: method `is_ascii_control` is not a member of trait `AsciiExt`
   --> libstd/ascii.rs:209:9
    |
209 |         fn is_ascii_control(&self) -> bool { self.is_ascii_control() }
    |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ not a member of trait `AsciiExt`
...
218 |     delegating_ascii_ctype_methods!();
    |     ---------------------------------- in this macro invocation

Do I want to remove the macro at all? I'm not quiet sure.

error[E0407]: method `is_ascii_hexdigit` is not a member of trait `AsciiExt`
   --> libstd/ascii.rs:261:5
    |
261 | /     fn is_ascii_hexdigit(&self) -> bool {
262 | |         self.iter().all(|b| b.is_ascii_hexdigit())
263 | |     }
    | |_____^ not a member of trait `AsciiExt`

Same here. I'm not sure, why this fails. Am I missing something?

@shepmaster
Copy link
Member

Ping from triage, @aturon !

@mattico
Copy link
Contributor

mattico commented Mar 3, 2018

I think all you have to do is remove the delegating_ascii_ctype_methods!() macro.

@hellow554
Copy link
Contributor Author

I'm sorry, but I cannot get this working. I'm not very sure what to do and which macro does what, because I'm not that deep into the codebase. I just tried to get rid of the feature flag in the doctests :)
A PR based on #39658 (comment) would be nice to see

@hellow554 hellow554 closed this Mar 5, 2018
@LukasKalbertodt
Copy link
Member

@hellow554 Sorry for replying so late, I didn't see this thread.

I wrote this macro to avoid boilerplate code and you should be able to remove it. I could offer some help/mentoring with the PR, if you want @hellow554? I'm by no means experienced in the compiler source, but I could help in this case, I think.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants