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
[WIP] Colorize function arguments and variables part 2 #10777
Conversation
If you have any idea on how to improve the code, please don't hesitate to tell it, I'm very unsure of some parts especially inside r_print_colorize_opcode |
ca97c64
to
c33d6f4
Compare
libr/util/print.c
Outdated
@@ -1629,6 +1631,26 @@ static bool issymbol(char c) { | |||
} | |||
} | |||
|
|||
static bool check_arg_name (char *p, RList *vars) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can u explain me why do we need this function? (i couldn't understand that's why asking)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So this function gets called from the r_colorize_opcode() loop when an arg of disasmed instruction is encountered. It returns true if it is equal to the name of one of the local vars.
I've added it to not clutter too much the r_colorize_opcode loop, but it is not necessary at all
r_list_join (list, reg_vars); | ||
r_list_join (list, bpv_vars); | ||
r_list_join (list, spv_vars); | ||
r_list_free (reg_vars); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good catch 👍
libr/util/print.c
Outdated
p[z] = '\0'; | ||
RAnalVar *var; | ||
RListIter *iter; | ||
r_list_foreach (vars, iter, var) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think instead of iterating through list of vars , us this function r_anal_var_get_byname
, it's faster since it uses sdb query to retrieve the var , also by doing this way we can avoid this hack and drop RList *vars
from RPrint
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey this is a great idea, it solves a lot of problems!
Now I'm going to sleep, tomorrow I'll modify it for sure.
Thanks :)
b6a6a46
to
54f91ed
Compare
RList *reg_vars = r_anal_var_list (anal, fcn, R_ANAL_VAR_KIND_REG); | ||
RList *bpv_vars = r_anal_var_list (anal, fcn, R_ANAL_VAR_KIND_BPV); | ||
RList *spv_vars = r_anal_var_list (anal, fcn, R_ANAL_VAR_KIND_SPV); | ||
r_list_join (list, reg_vars); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you can remove one of those lists if you just do RList *list = reg_vars; in the first line
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
RList *list is not an empty pointer, it is another list declared some lines before, that needs to be merged to reg_vars, bpv_vars, spv_vars...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
please fix the comment and rebase. should be green |
Use new colors for comments that hint type of data
fix whitespace
71833cc
to
6e04a55
Compare
ok! then ok to merge. but i would like to squash it, it is fine? |
…eorg#10777)" This PARTIALLY reverts commit 13a2cb3. All the updates to cons/d/* files were not reverted.
NOTE: I put it as [WIP] as this needs reviewing, there are some part which are a bit hackish
Changes in this pr:
Comments which indicate the type in disasm are colorized. However to do this, I was forced to add another
R_META_TYPE
, different from the normal comment, or ds_show_comments_right wouldn't be able to understand it was a special kind of comment that should be colorizedRenamed new colors from
func_arg*
tofunc_var*
, which makes more sense.Colorized local vars and args in disasm. This is the hackish bit. To achieve this, I had to pass the list of local vars to
r_print_colorize_opcode()
. I added a RList field to RPrint which gets populated with the vars. Then, inside r_print_colorize_opcode(), every argument is confronted with var names through strcmp. This is what needs reviewing the most: I fear this is slow, and not in the spirit of r2, passing variables around like this. But I really couldn't find a better way. Any help is appreciated.Manually modified each theme so that the new colors would fit with the style of each theme. This took some time, but it was worth it, r2 should be as beautiful as possible <3
Example screen of theme
sepia
:This works even with custom renamed vars, as you can see from the screen
custom_renamed_var
.Please if you have a bit of time cycle all colorschemes with
R
key to see if some colors mismatch (remember to use scr.color=3 though).