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.

Sign up for GitHub

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

Jump to bottom

Add LineNumberProgramHeader::raw_program_buf #168

Merged
merged 2 commits into from
Dec 13, 2016
Merged

Add LineNumberProgramHeader::raw_program_buf #168

merged 2 commits into from
Dec 13, 2016

Conversation

fitzgen
Copy link
Member

@fitzgen fitzgen commented Dec 13, 2016

r? @jonhoo

@fitzgen
Add LineNumberProgramHeader::raw_program_buf
637f70b 
This commit adds the `raw_program_buf` method to `LineNumberProgramHeader` to
get the underlying `EndianBuf` that contains the raw, un-parsed line number
program.
@fitzgen
Run rustfmt
20a441f
jonhoo
jonhoo reviewed Dec 13, 2016
View reviewed changes
src/unit.rs
@@ -2188,7 +2188,8 @@ impl<'input, 'abbrev, 'unit, Endian> EntriesCursor<'input, 'abbrev, 'unit, Endia
// down one level.
depth += 1;

let sibling_ptr = try!(self.current().unwrap().attr_value(constants::DW_AT_sibling));
let sibling_ptr =
try!(self.current().unwrap().attr_value(constants::DW_AT_sibling));
Copy link

Choose a reason for hiding this comment

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

This seems like a change that should be made separately?

Copy link
Member Author

Choose a reason for hiding this comment

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

Its in a second commit, which just ran rustfmt, so it is separate.

Copy link

Choose a reason for hiding this comment

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

Yeah, but I don't feel like it should be a part of this PR. In the larger context it probably doesn't matter, but this change could probably be pushed separately (maybe even without a PR)

@jonhoo
Copy link

jonhoo commented Dec 13, 2016

I don't think 20a441f should be included, but r+ for 637f70b

@coveralls
Copy link

Coverage Status

Coverage remained the same at 93.791% when pulling 20a441f on fitzgen:pub-program-buf into c0055fe on gimli-rs:master.

@fitzgen
Copy link
Member Author

fitzgen commented Dec 13, 2016

I don't think 20a441f should be included, but r+ for 637f70b

I think the best alternative to rustfmting on an as-needed basis in separate commits would be enforcing rustfmting of PRs in CI. In order to keep the workflow good, this would also require synchronizing on a particular rsutfmt version for all of us (perhaps enforced in the ./format script). Something we should probably do at some point, if we get enough concurrent contributors, but not something we need at the moment, when @philipc and I last discussed the issue.

@fitzgen fitzgen merged commit 907e81f into gimli-rs:master Dec 13, 2016
@jonhoo jonhoo mentioned this pull request Dec 13, 2016
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jonhoo jonhoo jonhoo left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@fitzgen @jonhoo @coveralls