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

librustc: Implement a Pod kind for types that can be memcpy'd. #10924

Merged
merged 4 commits into from
Dec 17, 2013

Conversation

pcwalton
Copy link
Contributor

This will be used for the new Cell.

r? @nikomatsakis

@nikomatsakis
Copy link
Contributor

Can you add a run-pass test like the following:

use std::kinds::Pod;

// Test that type parameters with the Pod bound are implicitly copyable.

fn can_copy_POD<T:Pod>(v: T) {
    let _a = v;
    let _b = v;
}

fn main() { }

@nikomatsakis
Copy link
Contributor

Also, should we put Pod in the prelude, like Send etc?

@pcwalton
Copy link
Contributor Author

Pod is already in the prelude.

@pcwalton
Copy link
Contributor Author

re-r? @nikomatsakis

@nikomatsakis
Copy link
Contributor

Given that POD is implicitly copyable, I think you can simplify Cell like so:

diff --git a/src/libstd/cell.rs b/src/libstd/cell.rs
index 6fb4ce4..582e989 100644
--- a/src/libstd/cell.rs
+++ b/src/libstd/cell.rs
@@ -12,6 +12,7 @@

 use prelude::*;
 use cast;
+#[cfg(stage0)]
 use unstable::intrinsics;
 use util::NonCopyable;

@@ -65,18 +66,15 @@ impl<T: ::kinds::Pod> Cell<T> {
     /// Returns a copy of the contained value.
     #[inline]
     pub fn get(&self) -> T {
-        unsafe {
-            let mut result = intrinsics::uninit();
-            intrinsics::copy_nonoverlapping_memory(&mut result, &self.value, 1);
-            result
-        }
+        self.value // because this is POD, we can copy it
     }

     /// Sets the contained value.
     #[inline]
     pub fn set(&self, value: T) {
         unsafe {
-            intrinsics::copy_nonoverlapping_memory(cast::transmute_mut(&self.value), &value, 1)
+            let p: &mut T = cast::transmute_mut(&self.value);
+            *p = value;
         }
     }
 }

@nikomatsakis
Copy link
Contributor

Also, I think that InteriorNonpod is not needed. It is only set when you own something owned anyway. So this patch should be equivalent I believe:

diff --git a/src/librustc/middle/ty.rs b/src/librustc/middle/ty.rs
index 43932c5..11cbf3a 100644
--- a/src/librustc/middle/ty.rs
+++ b/src/librustc/middle/ty.rs
@@ -1766,7 +1766,6 @@ def_type_content_sets!(

         // Things that are interior to the value (first nibble):
         InteriorUnsized                     = 0b0000__00000000__0001,
-        InteriorNonpod                      = 0b0000__00000000__0010,
         // InteriorAll                         = 0b0000__00000000__1111,

         // Things that are owned by the value (second and third nibbles):
@@ -1808,7 +1807,7 @@ def_type_content_sets!(
         Nonsized                            = 0b0000__00000000__0001,

         // Things that make values considered not POD
-        Nonpod                              = 0b0000__00001011__0010,
+        Nonpod                              = 0b0000__00001011__0000,

         // Bits to set when a managed value is encountered
         //
@@ -1882,8 +1881,7 @@ impl TypeContents {
          * Includes only those bits that still apply
          * when indirected through a `~` pointer
          */
-        TC::OwnsOwned |
-        TC::InteriorNonpod | (
+        TC::OwnsOwned | (
             *self & (TC::OwnsAll | TC::ReachesAll))
     }

@@ -2014,7 +2012,7 @@ pub fn type_contents(cx: ctxt, ty: t) -> TypeContents {
             }

             ty_estr(vstore_uniq) => {
-                TC::OwnsOwned | TC::InteriorNonpod
+                TC::OwnsOwned
             }

             ty_closure(ref c) => {
@@ -2133,7 +2131,7 @@ pub fn type_contents(cx: ctxt, ty: t) -> TypeContents {
                 match sigil {
                     ast::BorrowedSigil => TC::ReachesBorrowed,
                     ast::ManagedSigil => TC::Managed,
-                    ast::OwnedSigil => TC::OwnsOwned | TC::InteriorNonpod,
+                    ast::OwnedSigil => TC::OwnsOwned,
                 }
             }

@nikomatsakis
Copy link
Contributor

Regarding the prelude, I wasn't seeing it there, but that's probably because I only built rustc-stage1 or something.

@chris-morgan
Copy link
Member

At least a single mention of what POD stands for ("Plain Old Data", I am told) would be nice to have in the Pod doc comment.

@emberian
Copy link
Member

Manual needs to be updated, and ideally the error for non-pod would say why. But, rules in the manual are a fine substitute.

@pcwalton
Copy link
Contributor Author

Updated.

r? @nikomatsakis

@nikomatsakis
Copy link
Contributor

r+ if we resolve the matter of whether @T should be considered pod (I'm inclined to say no until GC is tracing, but I'd prefer to add a FIXME and link to #2997)

@thestinger
Copy link
Contributor

@nikomatsakis: even if it's tracing, it's not really POD as you're going to need some kind of special annotations + rooting API calls AFAICT

@nikomatsakis
Copy link
Contributor

On Mon, Dec 16, 2013 at 02:40:26AM -0800, Daniel Micay wrote:

@nikomatsakis: even if it's tracing, it's not really POD as you're going to need some kind of special annotations + rooting API calls AFAICT

I guess that will depend on precisely what annotations are required
and precisely what we mean by POD. I think we can end up with a scheme
where managed data is considered POD, but since we don't have one now
it shouldn't be included.

bors added a commit that referenced this pull request Dec 17, 2013
This will be used for the new `Cell`.

r? @nikomatsakis
@bors bors closed this Dec 17, 2013
@bors bors merged commit 8657017 into rust-lang:master Dec 17, 2013
flip1995 pushed a commit to flip1995/rust that referenced this pull request Jul 14, 2023
…r=Centri3,flip1995,Manishearth

Don't lint manual_let_else in cases where ? would work

Don't lint `manual_let_else` where the question mark operator `?` would be sufficient, that is, mostly in cases like:

```Rust
let v = if let Some(v) = ex { v } else { return None };
```

Also, this PR emits the `question_mark` lint for `let...else` patterns that could be written with `?` (also, only `return None` like cases).

```
changelog: [`manual_let_else`]: don't lint in cases where question_mark already lints
changelog: [`question_mark`]: lint for `let Some(...) = ex else { return None };`
```

Fixes  rust-lang#8755
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.

6 participants