Navigation Menu

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

Refactor types: extract API for ttc and tt ##refactor #659

Merged
merged 12 commits into from Feb 23, 2021

Conversation

officialcjunior
Copy link
Member

@officialcjunior officialcjunior commented Feb 19, 2021

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 added tests that prove my fix is effective or that my feature works (if possible)
  • I've updated the documentation and the rizin book with the relevant information (if needed)

Detailed description
This PR refactors a handful of places in cmd_type.c to use the API.
I've added two new RZ_APIs for the commands tt and ttc.
And they are:

RZ_API void rz_core_list_typename_alias_c(RzCore *core, const char *typedef_name);
RZ_API void rz_core_list_loaded_typedefs(RzCore *core, RzOutputMode mode);

Also added tests for ttj
Test plan
Firstly, we need to,

  • Name the APIs correctly and put them in the right place.

I'm not sure whether I've made tc working fine. I am currently looking into that.
But anyway, all of them have written unit tests, so if the CI succeeds, we can say that they're working fine.

Closing issues

None.

@codecov-io
Copy link

codecov-io commented Feb 19, 2021

Codecov Report

Merging #659 (3e8aee7) into dev (1fafb02) will decrease coverage by 0.01%.
The diff coverage is 79.71%.

Impacted file tree graph

@@            Coverage Diff             @@
##              dev     #659      +/-   ##
==========================================
- Coverage   42.80%   42.78%   -0.02%     
==========================================
  Files         869      869              
  Lines      316401   316410       +9     
==========================================
- Hits       135421   135366      -55     
- Misses     180980   181044      +64     
Impacted Files Coverage Δ
librz/core/cmd.c 44.51% <0.00%> (-0.02%) ⬇️
librz/core/cmd_type.c 69.65% <82.08%> (+2.47%) ⬆️
shlr/gdb/src/gdbclient/responses.c 15.03% <0.00%> (-18.43%) ⬇️
librz/debug/p/debug_gdb.c 48.19% <0.00%> (-12.14%) ⬇️
librz/reg/reg.c 77.77% <0.00%> (-5.26%) ⬇️
shlr/gdb/src/gdbclient/core.c 37.51% <0.00%> (-1.97%) ⬇️
librz/core/cmd_debug.c 27.33% <0.00%> (+0.06%) ⬆️
shlr/tcc/tccgen.c 39.76% <0.00%> (+0.08%) ⬆️
librz/util/sys.c 60.77% <0.00%> (+0.22%) ⬆️
librz/socket/socket.c 24.30% <0.00%> (+0.34%) ⬆️
... and 7 more

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 1fafb02...3e8aee7. Read the comment docs.

librz/core/cmd_type.c Outdated Show resolved Hide resolved
librz/core/cmd_type.c Outdated Show resolved Hide resolved
librz/core/cmd_type.c Outdated Show resolved Hide resolved
librz/core/cmd_type.c Outdated Show resolved Hide resolved
@XVilka XVilka added this to the 0.2.0 milestone Feb 20, 2021
@XVilka
Copy link
Member

XVilka commented Feb 20, 2021

Don't we have tests for those? Could you please add a couple? Will be helpful for the future RzTypes refactor as well.

} else if (rz_str_startswith(type, "func")) {
rz_core_cmdf(core, "tfc %s", name);
printFunctionTypeC(core, input + 3);
Copy link
Member

Choose a reason for hiding this comment

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

name here as well?

@officialcjunior
Copy link
Member Author

Don't we have tests for those? Could you please add a couple? Will be helpful for the future RzTypes refactor as well.

Added tests for tfc and tsc.
Do you know where I could find some enums and typenames/typedefs in the testbins, so that I could write tests for ttc and tuc?

@officialcjunior
Copy link
Member Author

Don't we have tests for those? Could you please add a couple? Will be helpful for the future RzTypes refactor as well.
Which commands were you referring to?

Actually, I just realized that tsc, tfc, tuc and tec already had tests in db/cmd/types, which are fr the ones I just added.
So, should I revert these changes or keep it?

@ret2libc
Copy link
Member

Please mark it as "ready to review" when we can have a look ;)

@officialcjunior officialcjunior marked this pull request as ready for review February 22, 2021 02:01
librz/core/cmd_type.c Outdated Show resolved Hide resolved
librz/core/cmd_type.c Outdated Show resolved Hide resolved
librz/core/cmd_type.c Outdated Show resolved Hide resolved
librz/core/cmd_type.c Outdated Show resolved Hide resolved
librz/core/cmd_type.c Outdated Show resolved Hide resolved
librz/core/cmd_type.c Outdated Show resolved Hide resolved
* Stop passing TDB, use core->analysis->sdb_types instead
* Changed the name of the argument to typedef_name
* Clean the `RZ_OUTPUT_MODE_JSON` case
librz/core/cmd_type.c Outdated Show resolved Hide resolved
librz/core/cmd_type.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.

One failing test:

[XX] db/cmd/types tt
RZ_NOPLUGINS=1 rizin -escr.utf8=0 -escr.color=0 -escr.interactive=0 -N -Qc 'td typedef char FILE_NAME;
tt FILE_NAME
"td typedef bool FLAG;"
tt FLAG
' -
-- stdout

@XVilka
Copy link
Member

XVilka commented Feb 23, 2021

It's because tt commands expects an argument, and refactoring removed that. See the test - tt FLAG should return bool, tt FILE_NAME should return char.

@XVilka XVilka merged commit c02f797 into rizinorg:dev Feb 23, 2021
@officialcjunior officialcjunior deleted the types-api branch February 23, 2021 10:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants