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

Implement AsRef and AsMut for str, [T] and some other unsized types #29

Merged
merged 2 commits into from
Feb 21, 2019

Conversation

mexus
Copy link
Contributor

@mexus mexus commented Sep 6, 2018

As discussed in #28

I've also made tests to make sure that we are getting what we really want to. On the other hand the functionality and implementation is so simple and straightforward, while the tests are a little bit clumsy (especially for AsMut<str>), so I'm hesitating on whether or not we should keep them.

@mexus
Copy link
Contributor Author

mexus commented Sep 15, 2018

Hi @cuviper ! Could you pls have a look at the PR?
I'm not really sure whom to invite to make a review (apart from you), maybe you have an idea whom else to ping?
Thanks!

@cuviper
Copy link
Member

cuviper commented Sep 18, 2018

I see what you mean about AsMut<str> being clumsy. Maybe we should take a cue from those unsized types that are explicitly implemented in the standard library:

  • AsRef<str>
  • AsRef<CStr>
  • AsRef<OsStr>
  • AsRef<Path>
  • AsRef<[T]>
  • AsMut<[T]>

So a few more candidates for AsRef, but none of those string-like types implement AsMut!

@cuviper
Copy link
Member

cuviper commented Sep 18, 2018

Oh, now I see you already improved the "clumsy" tests. I was thinking about the problem since you posted the PR, but hadn't checked to see that you updated it. That definitely looks nicer.

Now I guess it's just a question of what unsized types we really want.

@bluss, any opinions? In case you missed it, in #28 we found that going fully ?Sized would technically be a breaking change -- though it would probably not affect anyone in practice...

@mexus
Copy link
Contributor Author

mexus commented Sep 18, 2018

Omg, I've completely forgotten about other unsized types! xD
Do you think I should put implementations for them either?.. If yes, then I'd rather let people who need it for some specific standard types to make pull requests than to introduce another ton of copy-paste code into the library, taking into consideration that everything can be easily implemented with just a single question mark... and a breaking release :)

@cuviper
Copy link
Member

cuviper commented Sep 28, 2018

Macros can ease the copy-paste aspect of this, at least.

@mexus
Copy link
Contributor Author

mexus commented Oct 6, 2018

Added a couple of macros and implemented the traits in question for str, Path, OsStr and CStr

@mexus mexus changed the title Implement AsRef and AsMut for str and [T] Implement AsRef and AsMut for str, [T] and some other unsized types Oct 11, 2018
@bluss
Copy link
Contributor

bluss commented Oct 14, 2018

I'd love to do the ?Sized change, that's an oversight, but it's hard to see if it's possible to do. Generic is better than specific here.

Only PR comment is to update it so that it compiles with --no-default-features. Also an oversight that travis doesn't test that.

@mexus
Copy link
Contributor Author

mexus commented Oct 15, 2018

Will do later today, thanks for the hint!

@mexus
Copy link
Contributor Author

mexus commented Oct 15, 2018

Done!

Btw is it ok that I'm overwriting the same commit again and again? I can split the commits using my reflog if anybody finds it useful..

@mexus mexus force-pushed the master branch 2 times, most recently from f8f8360 to e14c347 Compare October 15, 2018 19:51
@mexus
Copy link
Contributor Author

mexus commented Oct 15, 2018

Just as a side question, why to maintain compatibility with rust-1.12?...

@cuviper
Copy link
Member

cuviper commented Oct 15, 2018

It's an open question in the Rust community. Some consider it fine to only support the latest-stable rustc, and some think increasing the rustc requirement should be a breaking change. This crate has chosen the more conservative position to maintain compatibility.

@mexus
Copy link
Contributor Author

mexus commented Oct 15, 2018

... yet another point to roll out 2.0 :))
(just kidding)

So, it there anything else to amend in the PR?

@mexus
Copy link
Contributor Author

mexus commented Dec 26, 2018

Hi @cuviper, @bluss
Do you think I can improve the patch somehow so it gets merged? :)

@cuviper cuviper merged commit 359e23d into rayon-rs:master Feb 21, 2019
@cuviper
Copy link
Member

cuviper commented Feb 21, 2019

Sorry for neglecting this. I've just merged and published it as either 1.5.1.

@mexus
Copy link
Contributor Author

mexus commented Feb 21, 2019

No problem, there was no hurry or anything :) Thanks!

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.

3 participants