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

Implemented offset annotation for comments #121

Merged
merged 3 commits into from Jun 29, 2020
Merged

Implemented offset annotation for comments #121

merged 3 commits into from Jun 29, 2020

Conversation

NirmalManoj
Copy link
Member

@NirmalManoj NirmalManoj commented Jun 26, 2020

Detailed description

Earlier we didn't have offset annotation (R_CODE_ANNOTATION_TYPE_OFFSET) for comments. Because of this, we have inconsistent behavior while clicking on a comment to edit in Cutter. This PR solves this problem by implementing offset annotations for comments.

The following GIFs show the previous behavior and improved behavior.
[Note: The following GIFs don't use the current master of Cutter. The same behavior can be noticed from the master also. So this PR can be merged after testing]

Before having offset annotation
BeforeCommentOffset
After implementing offset annotation
AfterCommentOffset_1

...

Test plan

  • Test with the current master of Cutter
  • Test if the changes are working as expected
  • Make sure nothing else is affected by this change

...

Closing issues

...

@karliss
Copy link
Member

karliss commented Jun 26, 2020

Broken test. It would also be good to test the offset for r2 comment instead of ghidra generated warning.

Copy link
Member

@karliss karliss left a comment

The code looks good just need to deal with tests.

@NirmalManoj
Copy link
Member Author

NirmalManoj commented Jun 26, 2020

@karliss Tests will likely work if we stop giving offset annotations for ghidra's warnings.

@karliss
Copy link
Member

karliss commented Jun 26, 2020

Tests will likely work if we stop giving offset annotations for ghidra's warnings.

Yes, but those offsets aren't exactly wrong. I think it's fine updating the test in this case. Is there even an easy way to distinguish source of comment?

@NirmalManoj
Copy link
Member Author

NirmalManoj commented Jun 26, 2020

It would also be good to test the offset for r2 comment instead of ghidra generated warning.

Is there even an easy way to distinguish source of comment?

I was looking to find a way to do this. Other than modifying some parts of the real ghidra that generate warnings, I couldn't find any easy way to do this. Even that will be sort of like a hack.

If we decide to leave it like this, this weird behavior will exist while clicking on a warning to add a comment. But as you said, I think it's okay as long as it won't affect users from adding/editing their own comments.

@NirmalManoj
Copy link
Member Author

NirmalManoj commented Jun 26, 2020

Is Jessie CI down? In PR #120, last commit was three days ago and Jessie's status is still Expected.

@NirmalManoj NirmalManoj self-assigned this Jun 28, 2020
@NirmalManoj NirmalManoj requested a review from karliss Jun 28, 2020
@karliss
Copy link
Member

karliss commented Jun 28, 2020

@NirmalManoj I am still waiting for the test which checks the offset of user created comment.

@NirmalManoj
Copy link
Member Author

NirmalManoj commented Jun 29, 2020

@karliss I have created a test that checks the offset of user-created comments.

Copy link
Member

@karliss karliss left a comment

Looks good.

I am sligtly confused by github actions "Jessie" build. It is configured to run on push not pull requests. Why is displayed here.

@thestr4ng3r thestr4ng3r merged commit d5c645f into rizinorg:master Jun 29, 2020
2 checks passed
@karliss karliss moved this from In progress to Done in Improving Decompiler Widget (GSoC) Jun 29, 2020
@NirmalManoj NirmalManoj moved this from Done to In progress in Improving Decompiler Widget (GSoC) Jul 3, 2020
@NirmalManoj NirmalManoj moved this from In progress to Done in Improving Decompiler Widget (GSoC) Jul 3, 2020
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.

None yet

3 participants