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

Convert rz_str_new to rz_str_dup #4178

Merged
merged 1 commit into from
Feb 6, 2024

Conversation

HN026
Copy link
Contributor

@HN026 HN026 commented Feb 1, 2024

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 #1176
...

librz/util/str.c Outdated Show resolved Hide resolved
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.

nice changes!

librz/core/tui/config.c Outdated Show resolved Hide resolved
librz/core/tui/panels.c Outdated Show resolved Hide resolved
librz/core/tui/panels.c Outdated Show resolved Hide resolved
librz/util/str.c Outdated Show resolved Hide resolved
librz/util/str.c Outdated Show resolved Hide resolved
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.

Good job! fix the declarations and it can be merged.

@XVilka
Copy link
Member

XVilka commented Feb 3, 2024

  1. ../librz/core/cmd/cmd_eval.c:100: error: identifier expected
  2. Please rebase on top of the latest dev

@HN026 HN026 force-pushed the convert_rz_str_new_to_rz_str_dup branch from 204c98d to 3179b9b Compare February 4, 2024 08:18
librz/core/cmd/cmd_eval.c Outdated Show resolved Hide resolved
Copy link
Member

@XVilka XVilka left a comment

Choose a reason for hiding this comment

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

Run python3 rz-bindgen/src/lint.py \
Translation unit diagnostic at <rizin/librz/core/cmd/cmd_eval.c:100:2>: expected expression
Translation unit diagnostic at <rizin/librz/core/cmd/cmd_eval.c:102:19>: use of undeclared identifier 'tmp'
Process completed with exit code 2.

Also formatting.

@HN026
Copy link
Contributor Author

HN026 commented Feb 4, 2024

  1. ../librz/core/cmd/cmd_eval.c:100: error: identifier expected

    1. Please rebase on top of the latest dev
Run python3 rz-bindgen/src/lint.py \
Translation unit diagnostic at <rizin/librz/core/cmd/cmd_eval.c:100:2>: expected expression
Translation unit diagnostic at <rizin/librz/core/cmd/cmd_eval.c:102:19>: use of undeclared identifier 'tmp'
Process completed with exit code 2.

Also formatting.

I've formatted all the files I've touched, Idk why it's breaking there.

@XVilka
Copy link
Member

XVilka commented Feb 4, 2024

See the CI logs for the error messages, formatting also.

@HN026
Copy link
Contributor Author

HN026 commented Feb 4, 2024

See the CI logs for the error messages, formatting also.

Sure

@HN026 HN026 force-pushed the convert_rz_str_new_to_rz_str_dup branch from 3179b9b to 711b603 Compare February 5, 2024 05:54
@Rot127
Copy link
Member

Rot127 commented Feb 6, 2024

@HN026 The error might come from a different clang-format version on your side. Currently we have clang-format-16 in the CI.

@HN026
Copy link
Contributor Author

HN026 commented Feb 6, 2024

@HN026 The error might come from a different clang-format version on your side. Currently we have clang-format-16 in the CI.

Yup I noticed will be pushing changes by evening.

@XVilka XVilka force-pushed the convert_rz_str_new_to_rz_str_dup branch from 86e950b to 33ab68f Compare February 6, 2024 14:38
@XVilka XVilka merged commit e82033d into rizinorg:dev Feb 6, 2024
44 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants