Review… #10
Merged
Review… #10
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The first paragraph of the documentation ends up as a summary line on rustdoc; having it be a single short sentence integrates well with this behaviour.
Clarifies what is being referenced from where, I think.
This binary is called thorin after all, so `RUST_DWP_LOG` seems quite arbitrary?
The `expect` in there looked quite out-of-place and I was going to replace it with a `match` over `self.$name`, before realizing this was “just” `get_or_insert_with`.
We are iterating over all of the sections of an input file, and while all section names we care about are UTF-8, we don't necessarily care that the other sections are valid UTF-8. This commit would allow us to handle those files correctly, still.
Accessing here may mean decompression, which can have a non-trivial cost. We're only interested in very specific sections, no need to try accessing all of them.
Having two distinct values representing the same information can lead to accidental divergence and be a source of bugs further down the line.
davidtwco
approved these changes
Dec 18, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The code looks good, at least given my understanding of DWARF which is definitely not super extensive.
I ended up with just a couple style nits and whatnot, best reviewed commit-by-commit.