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

support Errno formatting in no_std #43

Closed
wants to merge 1 commit into from
Closed

support Errno formatting in no_std #43

wants to merge 1 commit into from

Conversation

A1-Triard
Copy link
Contributor

No description provided.

src/lib.rs Show resolved Hide resolved
src/unix.rs Show resolved Hide resolved
src/windows.rs Show resolved Hide resolved
@A1-Triard A1-Triard closed this Apr 24, 2022
@lambda-fairy
Copy link
Owner

Hi @A1-Triard, sorry for the delay!

Any reason why this was closed? This looks pretty good to me.

@A1-Triard A1-Triard reopened this May 9, 2022
@A1-Triard
Copy link
Contributor Author

A1-Triard commented May 9, 2022

Hi! I had finally decided it will be better for me to stick with my own fork (errno-no-std, name is misleading now, but I don't want to pollute crates.io with multiply renamed copies). (This is not because the response delay, I have my own reasons.) That's the only reason, why it was closed.

In any case, I've reopened the request now. Please feel free to merge it if you want.

Copy link
Owner

@lambda-fairy lambda-fairy left a comment

Choose a reason for hiding this comment

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

LGTM modulo comments


Also, there's this error in CI:

error[E0433]: failed to resolve: use of undeclared type `CStr`
Error:   --> src/unix.rs:44:26
   |
44 |     let c_str = unsafe { CStr::from_ptr(buf.as_ptr()) };
   |                          ^^^^ use of undeclared type `CStr`

error[E0412]: cannot find type `c_char` in this scope
Error:   --> src/unix.rs:35:25
   |
35 |     let mut buf = [0 as c_char; 1024];
   |                         ^^^^^^
   |
help: a builtin type with a similar name exists
   |
35 |     let mut buf = [0 as char; 1024];
   |                         ~~~~
help: consider importing this type alias
   |
17 | use libc::c_char;
   |

error[E0425]: cannot find function `strerror_r` in this scope
Error:   --> src/unix.rs:37:12
   |
37 |         if strerror_r(err.0, buf.as_mut_ptr(), buf.len() as libc::size_t) < 0 {
   |            ^^^^^^^^^^ not found in this scope
   |
help: consider importing this function
   |
17 | use libc::strerror_r;
   |

for (c, i) in UStr::from_slice(input).char_indices_lossy().take_while(|&(x, _)| x != '\0') {
let c_len = c.len_utf8();
if c_len > output.len() {
return (i, unsafe { output.as_ptr().offset_from(output_start) } as usize);
Copy link
Owner

Choose a reason for hiding this comment

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

This unsafe looks unnecessary. Can we rewrite it using safe code? Say, by keeping track of how many bytes are written so far.

use winapi::um::winbase::{FORMAT_MESSAGE_FROM_SYSTEM, FORMAT_MESSAGE_IGNORE_INSERTS};

use Errno;

#[cfg(feature = "std")]
fn from_utf16_lossy(input: &[u16], mut output: &mut [u8]) -> (usize, usize) {
Copy link
Owner

Choose a reason for hiding this comment

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

This function guarantees that the written bytes are valid UTF-8. To localize this invariant, can we have the function return a &str directly?

fn from_utf16_lossy<'a>(input: &[u16], output: &'a mut [u8]) -> &'a str

src/lib.rs Show resolved Hide resolved
src/unix.rs Show resolved Hide resolved
@A1-Triard A1-Triard changed the base branch from main to allow-deprecated May 10, 2022 09:16
@A1-Triard A1-Triard changed the base branch from allow-deprecated to main May 10, 2022 09:16
@A1-Triard
Copy link
Contributor Author

Because I deleted the fork, I have to create a new pull request (#44) instead of this one. Please re-review it.

@A1-Triard A1-Triard closed this May 10, 2022
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.

2 participants