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

Pull actively info-applying functions out of rz_core_bin_info() #742

Merged
merged 8 commits into from
Mar 2, 2021

Conversation

thestr4ng3r
Copy link
Member

No description provided.

@rizinorg rizinorg deleted a comment from codecov bot Mar 1, 2021
@rizinorg rizinorg deleted a comment from codecov bot Mar 1, 2021
@codecov
Copy link

codecov bot commented Mar 1, 2021

Codecov Report

Merging #742 (f740f6f) into dev (2cefc50) will decrease coverage by 0.00%.
The diff coverage is 94.25%.

Impacted file tree graph

@@            Coverage Diff             @@
##              dev     #742      +/-   ##
==========================================
- Coverage   42.80%   42.79%   -0.01%     
==========================================
  Files         871      871              
  Lines      316703   316711       +8     
==========================================
- Hits       135554   135546       -8     
- Misses     181149   181165      +16     
Impacted Files Coverage Δ
librz/core/cmd_open.c 57.28% <0.00%> (ø)
librz/core/cconfig.c 88.58% <50.00%> (-0.05%) ⬇️
librz/core/cbin.c 85.87% <96.15%> (+0.24%) ⬆️
librz/bin/filter.c 66.80% <100.00%> (+0.14%) ⬆️
librz/core/cfile.c 66.22% <100.00%> (ø)
librz/core/cmd_info.c 75.60% <100.00%> (-0.07%) ⬇️
librz/core/disasm.c 77.47% <100.00%> (ø)
librz/socket/rzpipe.c 44.32% <0.00%> (-10.57%) ⬇️
librz/bin/bin.c 70.55% <0.00%> (-0.12%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2cefc50...b95ce9e. Read the comment docs.

@thestr4ng3r thestr4ng3r changed the title Some RzBin Refactoring Pull actively info-applying functions out of rz_core_bin_info() Mar 1, 2021
@thestr4ng3r thestr4ng3r marked this pull request as ready for review March 1, 2021 19:01
// use our internal values for va
va = va ? VA_TRUE : VA_FALSE;

rz_core_bin_apply_strings(r, binfile);
Copy link
Member

Choose a reason for hiding this comment

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

why all these calls?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is all the code that rz_core_bin_info(r, RZ_CORE_BIN_ACC_ALL, NULL, RZ_MODE_SET, va, NULL, NULL), which was there before was calling. The plan is to make it into individual functions rz_core_bin_apply_*() per info type and only for applying info, while rz_core_bin_info() will only do printing.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, YES! If this is to split the set and the printing, then yes please :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Exactly this.

static bool bin_raw_strings(RzCore *r, PJ *pj, int mode, int va);
static int bin_info(RzCore *r, PJ *pj, int mode, ut64 laddr);
static int bin_main(RzCore *r, PJ *pj, int mode, int va);
static int bin_dwarf(RzCore *core, PJ *pj, int mode);
Copy link
Member

Choose a reason for hiding this comment

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

Would it be possible to sort this list somehow?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, but these declarations are also only temporary until the are all unfolded and refactored.

Copy link
Member

Choose a reason for hiding this comment

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

ok

@thestr4ng3r
Copy link
Member Author

minstr/maxstr/filter is missing in apply_strings, putting this to draft again until I figured out how to do that best without code duplication.

@thestr4ng3r thestr4ng3r marked this pull request as draft March 1, 2021 20:53
@XVilka
Copy link
Member

XVilka commented Mar 2, 2021

It looks good. In the future we might consider to split librz/core/cmd_*.c files into the different directory than librz/core/cbin.c, librz/core/canalysis.c, or vice versa.

@Maijin
Copy link
Member

Maijin commented Mar 2, 2021

@thestr4ng3r if you're refactoring, may as well use the table API to have the ability to sort per column etc. 👍

@Maijin Maijin self-requested a review March 2, 2021 09:42
@thestr4ng3r
Copy link
Member Author

Table API is a good idea, but should be done seperately.

@thestr4ng3r thestr4ng3r marked this pull request as ready for review March 2, 2021 13:35
@thestr4ng3r thestr4ng3r enabled auto-merge (squash) March 2, 2021 13:37
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