Implement `Show` for `CString` and fix warning compiling tests for libcollections #15859

aochagavia · 2014-07-21T13:46:35Z

No description provided.

alexcrichton · 2014-07-21T15:32:52Z

When moving CString from libstd to librustrt, I purposely didn't move it to libcollections because it's more of a C-compatibility layer than a collection itself (in my opinion).

The CString API also explicitly has a dependency on libc for allocation/deallocation, which libcollections currently does not have and I would like to keep it that way because collections should not require a C runtime.

aochagavia · 2014-07-21T15:51:35Z

It makes sense that CString isn't really a collection. However, I don't think it belongs to librustrt either.

I guess the dependency of libcollections on libc is the biggest problem regarding this change. I don't see a clear solution for this.

alexcrichton · 2014-07-21T15:52:49Z

I don't see a clear solution for this.

Sadly that's why I just settled to put this in librustrt for now.

aochagavia · 2014-07-21T15:54:35Z

I guess I will just remove the first commit and go with the last two

aochagavia · 2014-07-21T16:12:50Z

Done. Thanks for reviewing!

aochagavia · 2014-07-21T18:14:28Z

Maybe this is bikeshedding, but... Have you considered moving CString to libc? I know the library contains bindings to types, constants and functions, but maybe we could add a fourth module called wrappers or such, where we can put CString (and eventually other types that may arise in the future).

alexcrichton · 2014-07-22T01:46:49Z

I considered that, but currently libc is purely just wrappers and I wasn't ready to jump the gun and push actual types into the crate. The module structure also didn't lend itself that well to the inclusion of official library types.

We don't currently have much precedent for Show on fallible-ish implementations, ala Path/RefCell. The rationale in the past has been that .to_string() is quite alluring. That being said, #[deriving(Show)] is a real pain if types don't implement Show.

I am personally in favor of implement Show as you have, but I believe that we may want to consider a more pervasive guideline for types such as this where technically Show can fail. cc @aturon on this topic

alexcrichton · 2014-07-22T01:47:19Z

src/librustrt/c_str.rs

@@ -344,6 +345,12 @@ impl Collection for CString {
    }
 }

+impl fmt::Show for CString {
+    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
+        write!(f, "{}", String::from_utf8_lossy(self.as_bytes_no_nul()))


This should be String::from_utf8_lossy(self.as_bytes_no_nul()).fmt(f) to preserve formatting specifiers and such.

We use use `from_utf8_lossy` to convert it to a MaybeOwned string, to avoid failing in case the CString contains invalid UTF-8

aochagavia · 2014-07-22T15:05:25Z

Just for clarity, fmt will fail if the buffer is the null pointer. This also happens in the current implementation of the Collection trait (see the len function for more details).

Additionaly, we have of course undefined behavior in case the buffer points to invalid memory and such. However, that should cause problems all over the place so I don't think it is something we have to deal with.

aturon · 2014-07-22T15:34:27Z

I think I'm OK with a failing Show here, because the only way to create a c_str is via an unsafe interface.

That said, I think we should probably revisit the c_str module as a whole. (For starters: it should probably be c_string.) It's a bit odd to me that new allows you to use a null pointer, but all the methods fail on null. Why not just have new fail on null, and avoid the checks/failing everywhere else? That would also take care of the issue with Show.

@alexcrichton Do you know of a rationale for the current design?

aochagavia · 2014-07-22T16:07:35Z

Currently, there is one place in the codebase where a new CString is made passing a null pointer. See libnative/io/addrinfo.rs on lines 30 and 31. It could be easily refactored.

alexcrichton · 2014-07-22T16:08:52Z

Sadly I don't know too, too much about the current design. All of your suggestions sound like a good direction to go in, however.

I was more concerned about the non-utf8 contents of CString and how from_utf8_lossy may yield surprising results from time to time. That being said, implementing Show for types is so crucial that I believe it's necessary to implement Show for CString regardless of whether its contents are utf8 or not. I then also think that format!("{}", foo) is so crucial to succeed, that there's no other choice than to implement it with from_utf8_lossy.

I suppose I am proposing this convention:

All types should implement Show. The Show implementation should strive to never fail!() and always produce valid utf-8 encoded contents.

aturon · 2014-07-22T16:15:43Z

@alexcrichton I think that's a useful convention; we should also then emphasize that Show is for debugging, while Encodable is used for (round-trippable) serialization.

aochagavia · 2014-07-22T16:29:09Z

@aturon I have added a check for null at CString::new and removed the rest of them.

I guess we can also deprecate the is_null and is_not_null methods. If somebody really needs them, it is still possible to call .as_ptr().is_null().

aturon · 2014-07-22T16:32:53Z

@aochagavia Can you also update the doc comments to reflect this change? The new method should have a # Failure section, and the # Failure section for the other methods should be removed.

alexcrichton · 2014-07-22T17:10:42Z

The commits will also need a [breaking-change] annotation as well, along with an explanation about why it is a breaking change and how to migrate.

aochagavia · 2014-07-22T17:54:08Z

I have included your suggestions. Should I also include [breaking-change] in the PR message?

I still have a question: why does libcuse i8 to represent c_char? I thought u8 was the obvious choice here...

aturon · 2014-07-22T22:10:57Z

@aochagavia It doesn't matter whether [breaking-change] is in the PR, just the commit (because the latter is searchable via git log | grep).

As to c_char, the standard does not specify signed/unsigned. See this thread for example. GCC and MSVC chars are signed.

aochagavia · 2014-07-23T08:02:47Z

Thanks for the link to stackoverflow!

The Travis build timed out, but everything should be ok.

alexcrichton · 2014-07-23T13:59:28Z

The last commit seems to have deleted the Show impl for CString, was that intentional?

aochagavia · 2014-07-23T19:12:21Z

That's pretty weird... I have fixed it now. Thanks!

This also removes checks in other methods of `CString` Breaking changes: * `CString::new` now fails if `buf` is null. To avoid this add a check before creatng a new `CString` . * The `is_null` and `is_not_null` methods are deprecated, because a `CString` cannot be null. * Other methods which used to fail if the `CString` was null do not fail anymore [breaking-change]

aochagavia · 2014-07-24T09:31:04Z

This should be ok now

alexcrichton · 2014-07-24T15:48:45Z

src/libnative/io/addrinfo.rs

-            let ch = if c_host.is_null() { null() } else { c_host.as_ptr() };
-            let cs = if c_serv.is_null() { null() } else { c_serv.as_ptr() };
+            let ch = if c_host.is_none() { null() } else { c_host.unwrap().as_ptr() };
+            let cs = if c_serv.is_none() { null() } else { c_serv.unwrap().as_ptr() };


This is a use-after-free because you're unwrapping c_host and c_serv, destroying their contents, and then referencing them right afterwards.

aochagavia mentioned this pull request Jul 21, 2014

Deprecate some unsafe functions in str::raw and remove OwnedStr trait #15807

Closed

aochagavia changed the title ~~Move c_str to collections and implement Show for CString~~ Implement Show for CString and fix warning compiling tests for libcollections Jul 21, 2014

alexcrichton reviewed Jul 22, 2014
View reviewed changes

aochagavia added 2 commits July 22, 2014 15:59

Fix deprecation warning in deque.rs

44fcec5

Implement Show for CString

3bfc852

We use use `from_utf8_lossy` to convert it to a MaybeOwned string, to avoid failing in case the CString contains invalid UTF-8

alexcrichton reviewed Jul 24, 2014
View reviewed changes

alexcrichton closed this in 1031ad8 Jul 24, 2014

aochagavia deleted the c_str branch October 13, 2014 18:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement `Show` for `CString` and fix warning compiling tests for libcollections #15859

Implement `Show` for `CString` and fix warning compiling tests for libcollections #15859

aochagavia commented Jul 21, 2014

alexcrichton commented Jul 21, 2014

aochagavia commented Jul 21, 2014

alexcrichton commented Jul 21, 2014

aochagavia commented Jul 21, 2014

aochagavia commented Jul 21, 2014

aochagavia commented Jul 21, 2014

alexcrichton commented Jul 22, 2014

alexcrichton Jul 22, 2014

aochagavia commented Jul 22, 2014

aturon commented Jul 22, 2014

aochagavia commented Jul 22, 2014

alexcrichton commented Jul 22, 2014

aturon commented Jul 22, 2014

aochagavia commented Jul 22, 2014

aturon commented Jul 22, 2014

alexcrichton commented Jul 22, 2014

aochagavia commented Jul 22, 2014

aturon commented Jul 22, 2014

aochagavia commented Jul 23, 2014

alexcrichton commented Jul 23, 2014

aochagavia commented Jul 23, 2014

aochagavia commented Jul 24, 2014

alexcrichton Jul 24, 2014

Implement Show for CString and fix warning compiling tests for libcollections #15859

Implement Show for CString and fix warning compiling tests for libcollections #15859

Conversation

aochagavia commented Jul 21, 2014

alexcrichton commented Jul 21, 2014

aochagavia commented Jul 21, 2014

alexcrichton commented Jul 21, 2014

aochagavia commented Jul 21, 2014

aochagavia commented Jul 21, 2014

aochagavia commented Jul 21, 2014

alexcrichton commented Jul 22, 2014

alexcrichton Jul 22, 2014

Choose a reason for hiding this comment

aochagavia commented Jul 22, 2014

aturon commented Jul 22, 2014

aochagavia commented Jul 22, 2014

alexcrichton commented Jul 22, 2014

aturon commented Jul 22, 2014

aochagavia commented Jul 22, 2014

aturon commented Jul 22, 2014

alexcrichton commented Jul 22, 2014

aochagavia commented Jul 22, 2014

aturon commented Jul 22, 2014

aochagavia commented Jul 23, 2014

alexcrichton commented Jul 23, 2014

aochagavia commented Jul 23, 2014

aochagavia commented Jul 24, 2014

alexcrichton Jul 24, 2014

Choose a reason for hiding this comment

Implement `Show` for `CString` and fix warning compiling tests for libcollections #15859

Implement `Show` for `CString` and fix warning compiling tests for libcollections #15859