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

Colorize function arguments #10752

Merged
merged 2 commits into from Jul 17, 2018
Merged

Colorize function arguments #10752

merged 2 commits into from Jul 17, 2018

Conversation

@cyanpencil
Copy link
Contributor

@cyanpencil cyanpencil commented Jul 16, 2018

Closes #10614

image

They are governed by config variable scr.color.args
Their palette names are func_arg, func_arg_type, func_arg_addr.

Please say here if you do not agree with the defaults I chose (I did not spend too much time selecting them), or you think they should be given a different name.

@radare
Copy link
Collaborator

@radare radare commented Jul 16, 2018

Update the themes

@XVilka
Copy link
Contributor

@XVilka XVilka commented Jul 17, 2018

@cyanpencil what about reusing the same color also in the comments which mark types of the argument:

│      ││   ; CODE XREFS from main (0x4013, 0x4977)                                                                                                                                                                                          
│      │└─> 0x00004049      488d3de84501.  lea rdi, ["COLUMNS"]        ; str.COLUMNS ; 0x18638 ; const char *name                                                                                                                            
│      │    0x00004050      48c705f5e121.  mov qword [0x00222250], 0x50    ; 'P' ; [0x222250:8]=0                                                                                                                                            
│      │    0x0000405b      e8d0f6ffff     call sym.imp.getenv         ;[1] ; char *getenv(const char *name)   

So this const char *name will use the same color as in the function beginning in the variables/arguments list? Like int local_48h? @radare @Maijin do you think it is a good idea?

@sivaramaaa
Copy link
Contributor

@sivaramaaa sivaramaaa commented Jul 17, 2018

0x00004049      488d3de84501.  lea rdi, ["COLUMNS"]        ; str.COLUMNS ; 0x18638 ; const char *name

But there is on problem though . not all type/name are mapped with variable, also the address where the comment is present and address where var is accesed could be different , so it would be difficult to find the address of instruction having this comment and map with the variables

@XVilka
Copy link
Contributor

@XVilka XVilka commented Jul 17, 2018

@sivaramaaa this is true, but I am not suggesting to map them somehow, I suggest just to use the same color from the theme

@sivaramaaa
Copy link
Contributor

@sivaramaaa sivaramaaa commented Jul 17, 2018

ah i get it , cool 👍

@radare
Copy link
Collaborator

@radare radare commented Jul 17, 2018

@cyanpencil
Copy link
Contributor Author

@cyanpencil cyanpencil commented Jul 17, 2018

Update the themes

@radare what do you mean by that? There are way too many themes to update manually, and I have no idea on which colors to use to be consistent with those...
Maybe I could do only the most common ones (the white / dark theme presets)

what about reusing the same color also in the comments which mark types of the argument

@XVilka Ok, will do it, shouldn't be hard. Do you think I should also color variable names in disasm with the same color as in the function beginning? Like local_48h in here:
| 0x000035df 4889442448 mov qword [local_48h], rax
which as of now is colored with the same color of the registers

@XVilka
Copy link
Contributor

@XVilka XVilka commented Jul 17, 2018

@cyanpencil make it white color for those themes you don't care, so ppl will send a fixes eventually.

Regarding variable names in the disasm - good idea too.

@radare
Copy link
Collaborator

@radare radare commented Jul 17, 2018

if you reuse another color theres no need to change all the themes

@cyanpencil
Copy link
Contributor Author

@cyanpencil cyanpencil commented Jul 17, 2018

I think @XVilka was suggesting the opposite: to reuse the new introduced colors func_* in disasm where necessary, not to reuse disasm colors in function arguments @radare

If you think we shouldn't introduce new colors at all, please state so bc then I'll have to change back a part of the pr

@radare
Copy link
Collaborator

@radare radare commented Jul 17, 2018

if func* ones are already there you are reusing them :P i just dont wanna have over 9000 options for colors, we must find a balance

@radare
Copy link
Collaborator

@radare radare commented Jul 17, 2018

nvm, do you want to update the themes in this PR?, as long as none have it yep it is a save merge. But we must have the themes updated before 2.8

@radare radare merged commit 8cb110e into radareorg:master Jul 17, 2018
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@XVilka
Copy link
Contributor

@XVilka XVilka commented Jul 18, 2018

@cyanpencil can you please use the same color in types and variable names as we discussed ?

@cyanpencil
Copy link
Contributor Author

@cyanpencil cyanpencil commented Jul 18, 2018

@XVilka yes I am currently working on it, but since it turned out to be a bit more complex I'll be doing that in a separate pr

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants