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

target/riscv: remove duplicate of progbufsize field #1125

Merged
merged 1 commit into from
Sep 6, 2024
Merged

target/riscv: remove duplicate of progbufsize field #1125

merged 1 commit into from
Sep 6, 2024

Conversation

fk-sc
Copy link
Contributor

@fk-sc fk-sc commented Sep 3, 2024 •
edited
Loading

Remove duplicate of progbufsize field

  • removed progbuf_size field from riscv_info; added getter
  • moved impebreak field from riscv_info to 'riscv013_info' as implementation dependent field; added getter

@fk-sc fk-sc force-pushed the fk-sc/field-duplication branch from e1bc8db to cf0806b Compare September 3, 2024 12:39
@fk-sc fk-sc changed the title Remove duplicate of progbufsize field target/riscv: remove duplicate of progbufsize field Sep 3, 2024
@fk-sc fk-sc force-pushed the fk-sc/field-duplication branch from cf0806b to 9ed55c2 Compare September 3, 2024 13:38
@fk-sc fk-sc changed the title target/riscv: remove duplicate of progbufsize field [NFC] target/riscv: remove duplicate of progbufsize field Sep 3, 2024
en-sc
en-sc previously approved these changes Sep 3, 2024
View reviewed changes
Copy link
Collaborator

@en-sc en-sc left a comment

Choose a reason for hiding this comment

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

LGTM (reviewed internally). @JanMatCodasip, @MarekVCodasip, please take a look.

JanMatCodasip
JanMatCodasip previously approved these changes Sep 4, 2024
View reviewed changes
Copy link
Collaborator

@JanMatCodasip JanMatCodasip left a comment

Choose a reason for hiding this comment

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

Thank you for the cleanup, it looks good.

I have posted few optional comments for your consideration, mostly related to code consistency.

src/target/riscv/riscv.h Show resolved Hide resolved
src/target/riscv/riscv-013.c Outdated Show resolved Hide resolved
src/target/riscv/program.c Outdated Show resolved Hide resolved
src/target/riscv/riscv.c Outdated Show resolved Hide resolved
src/target/riscv/riscv-013.c Show resolved Hide resolved
src/target/riscv/riscv-013.c Outdated Show resolved Hide resolved
MarekVCodasip
MarekVCodasip previously approved these changes Sep 4, 2024
View reviewed changes
Copy link
Collaborator

@MarekVCodasip MarekVCodasip left a comment

Choose a reason for hiding this comment

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

Nothing to add to @JanMatCodasip

@TommyMurphyTM1234
Copy link
Collaborator

Small (possibly dumb) question...
What does "[NFC]" in the PR title mean ?
The only thing that I can think of is "non-functional change" - meaning a code change that cleans something up but doesn't deliberately change the logic/functionality (although surely any code change impacts logic/functionality?).
Is that it or is it something else?
Thanks.

@fk-sc
Copy link
Contributor Author

fk-sc commented Sep 4, 2024

@TommyMurphyTM1234, you are right. NFC means non-functional change. I can rename PR if such naming is confusing

@TommyMurphyTM1234
Copy link
Collaborator

@TommyMurphyTM1234, you are right. NFC means non-functional change. I can rename PR if such naming is confusing

Thanks @fk-sc - I just hadn't come across such nomenclature before and couldn't find anything explanatory by searching. But maybe it's a commonly used shorthand.

@fk-sc fk-sc dismissed stale reviews from MarekVCodasip, JanMatCodasip, and en-sc via 441696f September 4, 2024 10:16
@fk-sc fk-sc force-pushed the fk-sc/field-duplication branch from 9ed55c2 to 441696f Compare September 4, 2024 10:16
@fk-sc fk-sc changed the title [NFC] target/riscv: remove duplicate of progbufsize field target/riscv: remove duplicate of progbufsize field Sep 4, 2024
@fk-sc fk-sc force-pushed the fk-sc/field-duplication branch from 441696f to 3f26c01 Compare September 4, 2024 12:19
MarekVCodasip
MarekVCodasip reviewed Sep 4, 2024
View reviewed changes
Copy link
Collaborator

@MarekVCodasip MarekVCodasip left a comment

Choose a reason for hiding this comment

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

I have two small nitpicks I additionally noticed. Please take a look.

src/target/riscv/riscv-013.c Show resolved Hide resolved
src/target/riscv/riscv.h Show resolved Hide resolved
@fk-sc
target/riscv: remove duplicate progbufsize field
a61e727 
* removed `progbuf_size`  field from `riscv_info`; added getter
* moved `impebreak` field from `riscv_info` to `riscv013_info`
  as implementation dependent field; added getter

Signed-off-by: Farid Khaydari <[email protected]>
@fk-sc fk-sc force-pushed the fk-sc/field-duplication branch from 3f26c01 to a61e727 Compare September 4, 2024 14:55
@fk-sc fk-sc requested a review from MarekVCodasip September 4, 2024 14:56
@en-sc en-sc merged commit d7a7c98 into riscv-collab:riscv Sep 6, 2024
4 checks passed
@fk-sc fk-sc deleted the fk-sc/field-duplication branch September 6, 2024 12:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MarekVCodasip MarekVCodasip MarekVCodasip approved these changes

@JanMatCodasip JanMatCodasip JanMatCodasip approved these changes

@en-sc en-sc Awaiting requested review from en-sc

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

Successfully merging this pull request may close these issues.
5 participants
@fk-sc @TommyMurphyTM1234 @JanMatCodasip @en-sc @MarekVCodasip