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

Use fmt machinery instead of io one #146

Merged
merged 2 commits into from
Jun 4, 2018
Merged

Conversation

est31
Copy link
Contributor

@est31 est31 commented Jun 4, 2018

This is in preparation of a #![no_std] port

This is in preparation of a #![no_std] port
@est31
Copy link
Contributor Author

est31 commented Jun 4, 2018

#![no_std] is needed for gimli-rs/addr2line#89 .

src/ast.rs Outdated
@@ -6021,7 +6015,8 @@ where
} else {
start
};
ctx.write_all(&ctx.input[start..end])
let s = ::std::str::from_utf8(&ctx.input[start..end]).map_err(|_| fmt::Error)?;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is this .map_err(|_| fmt::Error) okay? I'm doing this in multiple places in this PR. I could also create a custom error type or use the error type from the the error module of this crate (extending it with fmt::Error support previously).

Copy link
Member

Choose a reason for hiding this comment

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

See comment above.

@est31 est31 mentioned this pull request Jun 4, 2018
14 tasks
@fitzgen
Copy link
Member

fitzgen commented Jun 4, 2018

@est31 the motivation is that core::fmt exists and core::io does not, correct?

@est31
Copy link
Contributor Author

est31 commented Jun 4, 2018

@fitzgen exactly. We only use std::io in order to write stuff, so we can fully replace it with std::fmt usage, which exists inside core.

Copy link
Member

@fitzgen fitzgen left a comment

Choose a reason for hiding this comment

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

Thanks @est31 :)

One note inline below.

src/ast.rs Outdated
log_demangle!(self, ctx, scope);

let leaf = scope
.leaf_name()
.map_err(|e| io::Error::new(io::ErrorKind::Other, e))?;
.map_err(|_| fmt::Error)?;
Copy link
Member

Choose a reason for hiding this comment

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

For debuggability, we should do this:

.map_err(|e| {
    log!("Error getting leaf name: {}", e);
    fmt::Error
})

The log! will be a no-op when the logging feature is not enabled.

src/ast.rs Outdated
@@ -6021,7 +6015,8 @@ where
} else {
start
};
ctx.write_all(&ctx.input[start..end])
let s = ::std::str::from_utf8(&ctx.input[start..end]).map_err(|_| fmt::Error)?;
Copy link
Member

Choose a reason for hiding this comment

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

See comment above.

@fitzgen fitzgen merged commit 1d444ec into gimli-rs:master Jun 4, 2018
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