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

removed sdb_fmt in /arch/ #4123

Merged
merged 4 commits into from
Jan 19, 2024
Merged

removed sdb_fmt in /arch/ #4123

merged 4 commits into from
Jan 19, 2024

Conversation

HN026
Copy link
Contributor

@HN026 HN026 commented Jan 17, 2024 •
edited
Loading

Your checklist for this pull request

  • I've read the guidelines for contributing to this repository
  • I made sure to follow the project's coding style
  • I've documented or updated the documentation of every function and struct this PR changes. If not so I've explained why.
  • I've added tests that prove my fix is effective or that my feature works (if possible)
  • I've updated the rizin book with the relevant information (if needed)

Detailed description

...

Test plan

...

Closing issues

Partially addresses #1168
...

@HN026 HN026 requested a review from wargio as a code owner January 17, 2024 18:04
@HN026 HN026 changed the title removed sdb_fmt removed sdb_fmt in /arch/ Jan 17, 2024
@pelijah
Copy link
Contributor

pelijah commented Jan 17, 2024

IMO allocating strings on heap is not the best choice.

@HN026
Copy link
Contributor Author

HN026 commented Jan 17, 2024

IMO allocating strings on heap is not the best choice.

Other alternatives come w their trade-offs

@wargio
Copy link
Member

wargio commented Jan 18, 2024

IMO allocating strings on heap is not the best choice.

true but sdb_fmt uses an internal static char buffer[SIZE] which forbids us to do multithreading.

@wargio
Copy link
Member

wargio commented Jan 18, 2024

@pelijah i have added this change so @HN026 can use this instead of allocating mem #4124

@wargio
Copy link
Member

wargio commented Jan 18, 2024

@HN026 Please rebase and use the new setf_asm function

wargio
wargio requested changes Jan 19, 2024
View reviewed changes
Copy link
Member

@wargio wargio left a comment

Choose a reason for hiding this comment

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

Looks almost perfect besides for the issue in librz/asm/arch/6502/6502dis.c

librz/asm/arch/6502/6502dis.c Outdated Show resolved Hide resolved
librz/asm/arch/6502/6502dis.c Outdated Show resolved Hide resolved
@XVilka XVilka merged commit c33be9f into rizinorg:dev Jan 19, 2024
44 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wargio wargio wargio requested changes

@XVilka XVilka XVilka approved these changes

Assignees
No one assigned
Labels
RzAsm-Disassembler
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@HN026 @pelijah @wargio @XVilka