Replace C types with Rust types in std, closes #7313 #10943

fhahn · 2013-12-12T22:02:09Z

I've started working on a patch for #7313 . So far I tried to replace C types in src/libstd/unstable/* and related files.

So far, I have two questions. Is there a convention for passing pointers around in std as Rust types? Sometimes pointers are passed around as *c_char (which seems to be an *i8), *c_void or *u8, which leads to a lot of casts. E.g: exchange_malloc used to return a *c_char but the function in turn only calls malloc_raw which returns a *c_void.
Is there a specific reason for this?

The second question is about CString and related functions like with_c_str. At the moment these functions use *c_char*. Should I replace it with *u8 or keep it, because it's an wrapper around classical C strings?

brson · 2013-12-13T00:58:02Z

Thanks for working on this cleanup.

There's no specific reason for all the runtime functions returning weird combinations of pointer types, it's just the legacy of refactoring in various ways. Ideally, they all agree on the types and there are no casts.

CString should probably keep using c_char.

I think it's debatable whether the various logging and other runtime functions that deal in c strings should use *c_char or *u8 or some other typedef. This is all internal details so it doesn't matter too much. Ultimately we're probably going to want to be able to compile out std::libc in environments that don't have it, so the less we depend on it the better.

fhahn · 2013-12-13T17:44:01Z

So should various functions pass C strings around as u8? *c_str functions are used in a lot of places.

When working on this issue, another idea came to my mind. There probably are a couple of unnecessary type casts in the rust source code and while working on this issue, I accidentally introduced another. Would a lint for unnecessary type casts be useful in generel?

brson · 2013-12-18T23:59:45Z

@fhahn Public API's that deal with C strings should definitely use *c_char. The compiler interface (lang items like start and probably the failure stuff) should definitely not use *c_char. Other functions I'm less sure of but can probably be inferred from whether they are called form other functions that use *c_char.

brson · 2013-12-19T00:01:05Z

A lint for unnecessary type casts sounds useful, but I guess we won't know until we try.

fhahn · 2013-12-19T21:07:37Z

Bors failed to test this PR, because it could not be merged. I've rebased pull request to the current master.

emberian · 2013-12-25T00:16:51Z

@fhahn an "unneeded cast" lint sounds great.

fhahn · 2013-12-25T00:22:04Z

@cmr not sure if you already saw #11135, my first version of a lint for unnecessary casts. The main problem atm is handling macros which use casts.

fhahn · 2013-12-25T19:17:21Z

There was a missing type cast on Windows, which failed the build bot. It should be fixed now, but unfortunately I won't have a Windows (or OS X) installation handy over the next couple of days, to test the patch on other platforms than Linux.

fhahn · 2014-01-14T23:47:23Z

@brson no worries, I didn't have much time to work on this patch the last couple of days. Before this patch gets merged, I'd like to clear up a few points:

at the moment I'm not really sure if I should use *u8 or *Void instead of *c_void. I'd like to keep my patch consistent with the other parts (in the existing source sometimes *Void is used and sometimes *u8).
I think I made good progress with pushing libc types down, expect for pipes and processes. The problem with pipes is, that they store 2 file descriptors internally and are used in combination with the pre defined fds in libc, like stdout. If file descriptors of pipes are stored as a Rust type, then comparing with this predefined fds would require more additional typecasts or file descriptors as a Rust type.

adrientetar · 2014-01-16T12:29:44Z

src/libstd/os.rs

@@ -833,7 +834,7 @@ pub struct MemoryMap {
    /// Pointer to the memory created or modified by this map.
    data: *mut u8,
    /// Number of bytes this map applies to
-    len: size_t,
+    len: u64,


That should probably be uint, since 32-bit wants u32.

I've changed the type to uint and added a cast to size_t at the cast.

fhahn · 2014-01-16T23:09:42Z

I've updated the commit. It compiles on 64- and 32-bit linux.

fhahn · 2014-01-19T22:53:20Z

I've updated the commit again, it should compile on win32 now.

sfackler · 2014-01-21T23:15:35Z

@fhahn needs a rebase.

fhahn · 2014-01-21T23:26:16Z

@sfackler I'll look into it tomorrow. I managed to compile this patch on windows (win7 32 bit) without errors today, but 2 run-pass tests fail at the moment.

Following code compiles on Windows, but fails when building libstd in stage 1:

let nArgs: uint = 0;
let lpCmdLine = unsafe { GetCommandLineW() };
let szArgList = unsafe { CommandLineToArgvW(lpCmdLine, (&mut (nArgs as c_int)) as *mut c_int) };

I've reverted this change and I'm using the old code (with two c_int variables): https://github.com/mozilla/rust/blob/master/src/libstd/os.rs#L740

adrientetar · 2014-01-22T08:18:32Z

Needs a snapshot?

alexcrichton · 2014-01-22T17:52:00Z

src/libstd/rt/borrowck.rs

+    if (ref_count & FROZEN_BIT) != 0 {
+        fail_borrowed(a, file, line);
+    }
+}


I think this file came back from the dead!

alexcrichton · 2014-01-22T17:53:12Z

With the extra file removed, we can give this another go-through with bors.

fhahn · 2014-01-22T18:45:34Z

I've removed the file again and squashed everything in one commit. I think another try with bors would be good.

…richton I've started working on a patch for #7313 . So far I tried to replace C types in `src/libstd/unstable/*` and related files. So far, I have two questions. Is there a convention for passing pointers around in `std` as Rust types? Sometimes pointers are passed around as `*c_char` (which seems to be an `*i8`), `*c_void` or `*u8`, which leads to a lot of casts. E.g: [`exchange_malloc`](https://github.com/fhahn/rust/compare/issue-7313-replace-c-types?expand=1#diff-39f44b8c3f4496abab854b3425ac1617R60) used to return a `*c_char` but the function in turn only calls `malloc_raw` which returns a `*c_void`. Is there a specific reason for this? The second question is about `CString` and related functions like `with_c_str`. At the moment these functions use `*c_char*`. Should I replace it with `*u8` or keep it, because it's an wrapper around classical C strings?

[`map_identity`]: recognize tuple identity function Fixes rust-lang#7189 This lint now recognizes `.map(|(a, b)| (a, b))` as a useless `map` call. changelog: [`map_identity`]: recognize tuple identity function

adrientetar reviewed Jan 16, 2014
View reviewed changes

alexcrichton reviewed Jan 22, 2014
View reviewed changes

Replace C types with Rust types in libstd, closes rust-lang#7313

2eb4f05

bors merged commit 2eb4f05 into rust-lang:master Jan 22, 2014

fhahn deleted the issue-7313-replace-c-types branch January 23, 2014 10:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace C types with Rust types in std, closes #7313 #10943

Replace C types with Rust types in std, closes #7313 #10943

fhahn commented Dec 12, 2013

brson commented Dec 13, 2013

fhahn commented Dec 13, 2013

brson commented Dec 18, 2013

brson commented Dec 19, 2013

fhahn commented Dec 19, 2013

emberian commented Dec 25, 2013

fhahn commented Dec 25, 2013

fhahn commented Dec 25, 2013

fhahn commented Jan 14, 2014

adrientetar Jan 16, 2014

fhahn Jan 16, 2014

fhahn commented Jan 16, 2014

fhahn commented Jan 19, 2014

sfackler commented Jan 21, 2014

fhahn commented Jan 21, 2014

adrientetar commented Jan 22, 2014

alexcrichton Jan 22, 2014

alexcrichton commented Jan 22, 2014

fhahn commented Jan 22, 2014

Replace C types with Rust types in std, closes #7313 #10943

Replace C types with Rust types in std, closes #7313 #10943

Conversation

fhahn commented Dec 12, 2013

brson commented Dec 13, 2013

fhahn commented Dec 13, 2013

brson commented Dec 18, 2013

brson commented Dec 19, 2013

fhahn commented Dec 19, 2013

emberian commented Dec 25, 2013

fhahn commented Dec 25, 2013

fhahn commented Dec 25, 2013

fhahn commented Jan 14, 2014

adrientetar Jan 16, 2014

Choose a reason for hiding this comment

fhahn Jan 16, 2014

Choose a reason for hiding this comment

fhahn commented Jan 16, 2014

fhahn commented Jan 19, 2014

sfackler commented Jan 21, 2014

fhahn commented Jan 21, 2014

adrientetar commented Jan 22, 2014

alexcrichton Jan 22, 2014

Choose a reason for hiding this comment

alexcrichton commented Jan 22, 2014

fhahn commented Jan 22, 2014