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

some tiny improvements and way more comments #162

Open
wants to merge 5 commits into
base: develop
Choose a base branch
from

Conversation

SimonWaldherr
Copy link

pull request changed to "develop"-branch as requested by @will-v-pi (#160)

Copy link
Contributor

@will-v-pi will-v-pi left a comment

Choose a reason for hiding this comment

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

Thanks, these changes look good - there are a couple things I'd want changed, but probably worth a review from @lurch too?

elf2uf2/elf2uf2.cpp Show resolved Hide resolved
elf2uf2/elf2uf2.cpp Outdated Show resolved Hide resolved
elf2uf2/elf2uf2.cpp Outdated Show resolved Hide resolved
elf2uf2/elf2uf2.cpp Outdated Show resolved Hide resolved
elf2uf2/elf2uf2.cpp Outdated Show resolved Hide resolved
* @param vaddr Virtual address.
* @param size Size of the area to check.
* @param uninitialized Flag indicating if the area is uninitialized.
* @param ar Reference to an address_range that will be set on success.
Copy link
Contributor

Choose a reason for hiding this comment

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

"Reference to the valid_range that the provided (addr + size) fits inside" ?

elf2uf2/elf2uf2.cpp Outdated Show resolved Hide resolved
elf2uf2/elf2uf2.cpp Outdated Show resolved Hide resolved
*
* @param entries Vector of ELF-32 program header entries.
* @param valid_ranges Valid address ranges.
* @param pages Map of pages with their fragments.
Copy link
Contributor

Choose a reason for hiding this comment

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

"Reference to the page fragments map", to make it more obvious that this is "the page fragments map" to which the @brief is referring?

elf2uf2/elf2uf2.cpp Outdated Show resolved Hide resolved
Comment on lines +342 to +343
* @param package_addr Optional packaging address.
* @param abs_block_loc Optional absolute block location.
Copy link
Contributor

Choose a reason for hiding this comment

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

Same questions about "Optional"

elf2uf2/elf2uf2.cpp Outdated Show resolved Hide resolved
@SimonWaldherr
Copy link
Author

So, what's the status? Would you like any other changes?

@will-v-pi
Copy link
Contributor

There are a couple of unresolved comments from @lurch still - but once those are resolved this can be merged

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.

3 participants