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

Move replace and swap to std::mem. Get rid of std::util #11956

Merged
merged 1 commit into from
Feb 11, 2014

Conversation

edwardw
Copy link
Contributor

@edwardw edwardw commented Jan 31, 2014

Closes #7556.

Also move std::util::Void to std::any::Void. It makes more sense to me.

@alexcrichton
Copy link
Member

Everywhere except libstd implicitly has use std::prelude::* at the top, so could you remove all the imports of use std::prelude::{...}?

@edwardw
Copy link
Contributor Author

edwardw commented Jan 31, 2014

Sure thing. Will do.

@edwardw
Copy link
Contributor Author

edwardw commented Feb 1, 2014

Fixed and rebased, please review the change.

@edwardw
Copy link
Contributor Author

edwardw commented Feb 3, 2014

r? @alexcrichton

@alexcrichton
Copy link
Member

I'm slightly uncomfortable with how this turned out, having all these magical functions in scope automatically seems... interesting.

I'd be curious what others think about this, but regardless I'll add this to the meeting agenda.

@edwardw
Copy link
Contributor Author

edwardw commented Feb 3, 2014

Exactly! It does look a little bit strange to have functions come from nowhere. At the very least the std::util::Void => std::any::Void part makes sense to me, though.

@sfackler
Copy link
Member

sfackler commented Feb 4, 2014

@alexcrichton util::ignore moving to prelude::drop felt a bit weird for the same reason.

@edwardw
Copy link
Contributor Author

edwardw commented Feb 6, 2014

What's the verdict then?

@alexcrichton
Copy link
Member

@brson and I were discussing this the other day, and we came to the conclusion that drop, swap, and replace are core language features that are the building blocks upon which you create data structures to deal with ownership.

As a consequence, I would be in favor of this.

@brson
Copy link
Contributor

brson commented Feb 9, 2014

There are a few things that are now hidden away in intrinsics that we might drop into std::mem: init, uninit and move_val_init, the byte swapping functions like to_le16. It seems like swap and replace could also fit in there, leaving only id and Void.

@thestinger I'm curious what you think.

@brson
Copy link
Contributor

brson commented Feb 9, 2014

Oh, I didn't realize this branch dealt with both id and Void, nicely.

@edwardw
Copy link
Contributor Author

edwardw commented Feb 9, 2014

IMHO moving swap and replace to std::mem would make perfect sense. The rust-doc of std::mem currently reads:

Functions relating to memory layout

We may want to change that a little bit if those functions are going to be landed there.

@edwardw
Copy link
Contributor Author

edwardw commented Feb 9, 2014

Rebased again and got rid of std::util altogether. The patch is quite satisfactory to my eye now.

r? @alexcrichton @thestinger

bors added a commit that referenced this pull request Feb 9, 2014
As mentioned #11956 (comment) I've taken some of the most commonly-used intrinsics and put them in a more logical place, reduced the amount of code looking in `unstable::intrinsics`.

r? @thestinger
@alexcrichton
Copy link
Member

r+ from me, but I'd like another confirmation on destruction of std::util

@edwardw
Copy link
Contributor Author

edwardw commented Feb 10, 2014

r? @thestinger

@huonw
Copy link
Member

huonw commented Feb 10, 2014

Needs a rebase.

@huonw
Copy link
Member

huonw commented Feb 10, 2014

Also, it would be good to update the PR description with the final outcome of this re-organisation, because @bors copies that text into the git history.

@edwardw
Copy link
Contributor Author

edwardw commented Feb 10, 2014

Rebased and the PR description updated. Please re-review.
r? @huonw

@edwardw
Copy link
Contributor Author

edwardw commented Feb 10, 2014

I accidentally found that some commits, e.g. #12143, had been lost in the various rebases of this patch. Please advise how to proceed.

@edwardw
Copy link
Contributor Author

edwardw commented Feb 10, 2014

Turns out it needs yet another rebase. I'll inform this thread when it is done.

@edwardw
Copy link
Contributor Author

edwardw commented Feb 10, 2014

The rebase is done. Hope this time it'll make to the master.
r? @huonw

match self {
// Nothing to match on
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I realize you simply moved this implementation, but I don't understand the point of uninhabited(). When would you ever call it? I don't see any calls in the rust repo.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I honestly don't know. I'd suggest to handle it in another (new) ticket.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Based on the IRC conversation, and the fact that there are no callers in the rust repo, I would suggest deleting this method outright. It doesn't seem useful, and the docstring is certainly wrong. It can be re-added if someone comes up with a reason for its existence.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, I'll do it.

@lilyball
Copy link
Contributor

As part of this PR, I think std::prelude::drop should be moved into std::mem (and re-exported in prelude). It only lives in prelude for lack of a good home, but prelude really should just be limited to re-exports.

swap and replace should remain un-re-exported though. Let callers use std::mem.

@lilyball
Copy link
Contributor

Looks good at this point, r+ from me, except I think that 3 of the last 4 commits (removing uninhabited, tidying up prelude, removing impl Void {}) should be squashed into the (appropriate) earlier commits before merging.

@edwardw
Copy link
Contributor Author

edwardw commented Feb 10, 2014

So they were squashed.

Also move Void to std::any, move drop to std::mem and reexport in
prelude.
bors added a commit that referenced this pull request Feb 10, 2014
Closes #7556.

Also move ``std::util::Void`` to ``std::any::Void``. It makes more sense to me.
@bors bors closed this Feb 11, 2014
@bors bors merged commit e9ff91e into rust-lang:master Feb 11, 2014
@edwardw edwardw deleted the issue-7556 branch February 11, 2014 06:50
flip1995 pushed a commit to flip1995/rust that referenced this pull request Dec 16, 2023
…arenthesis, r=Alexendoo

[`doc_markdown`] Recognize words followed by empty parentheses `()` for quoting

*Please write a short comment explaining your change (or "none" for internal only changes)*

changelog: [`doc_markdown`] Recognize words followed by empty parentheses for quoting, e.g. `func()`.

---

Developers often write function/method names with trailing `()`, but `doc_markdown` lint did not consider that.

Old clippy suggestion was not very good:

```patch
-/// There is no try (do() or do_not()).
+/// There is no try (do() or `do_not`()).
```

New behavior recognizes function names such as `do()` even they contain no `_`/`::`; and backticks are suggested outside of the `()`:

```patch
-/// There is no try (do() or do_not()).
+/// There is no try (`do()` or `do_not()`).
```
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.

Get rid of the std::util module
8 participants