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

Improve labels and numeric syntax highlighting #848

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

am11
Copy link
Contributor

@am11 am11 commented Oct 11, 2021

Before After
2021-10-12_329x440_scrot 2021-10-12_310x408_scrot

Notes on differences:
* colon after label on left side are not classified as tag anymore.

  • labels on right side (after jump instructions) are now classified as tag.
  • decimal and hexadecimal (addresses) numbers are classified as string.

@ashmind
Copy link
Owner

ashmind commented Oct 12, 2021

Thanks for your work!

I am bit on a fence with those -- will add some questions and comments.
(no rush on those, I probably can only take second look later this week)

  • Question: The importance of colon seems to be the same as importance of label. What is the main benefit of treating it separately?
  • Comment: Current IL highlighting seems to only do left-side as a tag as well. I kind of understand it, because left side is mostly ignorable/optional, but on right side it plays an important role.
  • Question: Hex numbers are still numbers. What is the main benefit of highlighting them as strings?

@am11
Copy link
Contributor Author

am11 commented Oct 12, 2021

Question: The importance of colon seems to be the same as importance of label. What is the main benefit of treating it separately?

Actually it was a (minor) consequence of change, not something I was feeling strongly about, I'll include colon back to the tag class. :)

Comment: Current IL highlighting seems to only do left-side as a tag as well. I kind of understand it, because left side is mostly ignorable/optional, but on right side it plays an important role.

I was looking at assembly highlighting in godbolt (which goes easy on eyes) and got the idea from there that since label on the left and right are corelated, they can use the same color.

BTW, in godbolt (e.g. https://godbolt.org/z/24H4SZ), labels on the right are also anchored and clicking on label on right highlights the corresponding one on the left. However, it is not a smoothest experience yet, as if the target is out of visible window clicking on R.H.S label does not scroll-to target.

Perhaps we can also consider adding permalinks / line numbers / internal links etc. in the future to make assembly code more interesting / palatable.

Question: Hex numbers are still numbers. What is the main benefit of highlighting them as strings?

All numbers (hex or decimal) used in operands (constants, addresses etc.) are highlighted alike. This gist had many interesting cases where I believe someone reading the assembly may find newer / highlighted version more user friendly: https://sharplab.io/#gist:76692d6244e43c9700b0bcd8ed8e5446.

Interestingly, GitHub linguist also highlight ```asm in similar fashion:

    L0103: jmp L0247
    L0108: mov ecx, 1
    L010d: call 0x00007ff8f4d90570
    L0112: cmp eax, 1
    L0115: je short L0121
    L0117: mov eax, 6
    L011c: jmp L0247
    L0121: mov dword ptr [rbp-0x10], 1
    L0128: lea rcx, [rbp-0x10]
    L012c: call 0x00007ff8f4d90590
    L0131: cmp dword ptr [rbp-0x10], 2
    L0135: je short L0141
    L0137: mov eax, 7
    L013c: jmp L0247
    L0141: mov dword ptr [rsp+0x20], 5
    L0149: mov dword ptr [rsp+0x28], 6
    L0151: mov dword ptr [rsp+0x30], 7
    L0159: mov dword ptr [rsp+0x38], 8
    L0161: mov ecx, 1
    L0166: mov edx, 2
    L016b: mov r8d, 3
    L0171: mov r9d, 4
    L0177: call 0x00007ff8f4d905b0
    L017c: cmp eax, 0x24
    L017f: je short L018b

@am11
Copy link
Contributor Author

am11 commented Oct 12, 2021

label colons are back in cm-tag:

2021-10-12_326x441_scrot

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

Successfully merging this pull request may close these issues.

2 participants