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

Documentation + Clean up #2374

Merged
merged 14 commits into from Aug 14, 2020
Merged

Conversation

NirmalManoj
Copy link
Member

@NirmalManoj NirmalManoj commented Aug 6, 2020

Your checklist for this pull request

Detailed description
This PR make the following changes.

  1. Use the line offset as a fallback in case the cursor is on something that isn't directly associated with any offset.
  2. Make isReference() function.
  3. Renamed some things and formatted.
  4. Document functions in the DecompilerWidget and the DecompilerContextMenu.

Note: Fetch decompiler-refactoring branches in radare2 and r2ghidra-dec. Don't compile with bundled radare2 for testing this PR.

Test plan (required)

  1. Check if everything is working properly.
  2. Read code, compile documentation, and see how it looks.
  3. I haven't documented all functions in DecompilerContextMenu yet. I am a bit confused as to if I should document them or if they are not worthy of documentation as what they do is evident from the name. If you need me to add documentation for any other functions, please suggest them.

Closing issues

@NirmalManoj NirmalManoj requested review from ITAYC0HEN, karliss and thestr4ng3r and removed request for ITAYC0HEN and karliss Aug 6, 2020
@NirmalManoj NirmalManoj self-assigned this Aug 6, 2020
@NirmalManoj NirmalManoj added the Decompiler Issues and feature requests related to the decompiler in Cutter label Aug 6, 2020
@NirmalManoj
Copy link
Member Author

NirmalManoj commented Aug 6, 2020

CI fails because of the sdb.h not found problems. Hopefully, this will get fixed by tomorrow night as Yossi is working on it.

QString curHighlightedWord;
RVA offset;
RVA decompiledFunctionAddress;
/**
* First offset in the line under cursor in the decompiler widget.
Copy link
Member

@ITAYC0HEN ITAYC0HEN Aug 7, 2020

Choose a reason for hiding this comment

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

Which offset is considered the "first offset"? The lowest one in them? The one that is treated first in the current line?

Copy link
Member Author

@NirmalManoj NirmalManoj Aug 7, 2020

Choose a reason for hiding this comment

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

Explained in docs.

/**
* List of the offsets of all the breakpoints that are present in the line under cursor.
*/
Copy link
Member

@ITAYC0HEN ITAYC0HEN Aug 7, 2020

Choose a reason for hiding this comment

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

Only enabled breakpoints or also disabled ones?

(see Breakpoints widget, you can have disabled breakpoints)

Copy link
Member Author

@NirmalManoj NirmalManoj Aug 7, 2020

Choose a reason for hiding this comment

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

Clarified that this refers to both enabled and disabled breakpoints in docs.


/**
* Context-related annotation for the data under cursor in the decompiler widget.
* If such an annotation doesn't exist, it's value is nullptr.
Copy link
Member

@ITAYC0HEN ITAYC0HEN Aug 7, 2020

Choose a reason for hiding this comment

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

Suggested change
* If such an annotation doesn't exist, it's value is nullptr.
* If such an annotation doesn't exist, its value is nullptr.

Copy link
Member Author

@NirmalManoj NirmalManoj Aug 7, 2020

Choose a reason for hiding this comment

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

Changed as suggested.

Copy link
Member

@ITAYC0HEN ITAYC0HEN left a comment

I'd change thigns like "is a reference" to something which makes it clear what "a reference" here. You did it once, when you detailed the things which are considered a reference like "function name, global variable, ....". So waht I'd do is trying to find better words to describe it, like "is referencing an addressable object" or "is referencing an item that has an address assigned to it".

@@ -94,6 +109,12 @@ private slots:
QAction actionSetPC;

// Private Functions
/**
* @brief Sets the shortcut context in all the action contained
Copy link
Member

@ITAYC0HEN ITAYC0HEN Aug 7, 2020

Choose a reason for hiding this comment

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

Suggested change
* @brief Sets the shortcut context in all the action contained
* @brief Sets the shortcut context in all the actions contained

Copy link
Member Author

@NirmalManoj NirmalManoj Aug 7, 2020

Choose a reason for hiding this comment

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

Changed as suggested.

@@ -123,9 +144,32 @@ private slots:
void addBreakpointMenu();
void addDebugMenu();

/**
* @brief Updates targeted show in menu if annotationHere
Copy link
Member

@ITAYC0HEN ITAYC0HEN Aug 7, 2020

Choose a reason for hiding this comment

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

Suggested change
* @brief Updates targeted show in menu if annotationHere
* @brief Updates targeted "show in" menu if annotationHere

Copy link
Member Author

@NirmalManoj NirmalManoj Aug 7, 2020

Choose a reason for hiding this comment

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

Changed as suggested.

/**
* @brief Updates targeted show in menu if annotationHere
* represents a reference.
*/
void updateTargetMenuActions();
Copy link
Member

@ITAYC0HEN ITAYC0HEN Aug 7, 2020

Choose a reason for hiding this comment

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

the name of the function doesn't make it clear that it only applied if the annotation represents a reference

Copy link
Member Author

@NirmalManoj NirmalManoj Aug 7, 2020

Choose a reason for hiding this comment

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

Actually, this function removes all the actions in the previous targeted show in menu even if it doesn't represent a reference. I will add that to the documentation.

void updateTargetMenuActions();

/**
* @brief Check if annotationHere is a reference(function name,
Copy link
Member

@ITAYC0HEN ITAYC0HEN Aug 7, 2020

Choose a reason for hiding this comment

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

Suggested change
* @brief Check if annotationHere is a reference(function name,
* @brief Check if annotationHere is a reference (function name,

Copy link
Member Author

@NirmalManoj NirmalManoj Aug 7, 2020

Choose a reason for hiding this comment

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

Changed as suggested.

void updateTargetMenuActions();

/**
* @brief Check if annotationHere is a reference(function name,
* global variable, constant variable with address).
Copy link
Member

@ITAYC0HEN ITAYC0HEN Aug 7, 2020

Choose a reason for hiding this comment

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

Suggested change
* global variable, constant variable with address).
* global variable, constant variable with an address).

Copy link
Member Author

@NirmalManoj NirmalManoj Aug 7, 2020

Choose a reason for hiding this comment

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

Changed as suggested.

void *annotationi;
r_vector_foreach(&codeDecompiled.annotations, annotationi) {
r_vector_foreach(&code->annotations, annotationi) {
Copy link
Member

@ITAYC0HEN ITAYC0HEN Aug 7, 2020

Choose a reason for hiding this comment

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

Why the "i" in the end for "annotationi"?

Copy link
Member Author

@NirmalManoj NirmalManoj Aug 7, 2020

Choose a reason for hiding this comment

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

I have replaced void *annotationi with void *iter to avoid any unnecessary confusion. It's just a random name for the void pointer to use in r_vector_foreach.

void copy();
void fontsUpdatedSlot();
void colorsUpdatedSlot();
void refreshDecompiler();
void decompilerSelected();
void cursorPositionChanged();
/**
* @brief When the synced seek is changed, this refreshes the decompiler
* if needed
Copy link
Member

@ITAYC0HEN ITAYC0HEN Aug 7, 2020

Choose a reason for hiding this comment

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

if needed? tell us how you know when it is needed and when it is not

Copy link
Member Author

@NirmalManoj NirmalManoj Aug 7, 2020

Choose a reason for hiding this comment

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

Explained.

void setAutoRefresh(bool enabled);
/**
* @brief Do refersh if the auto refresh is enabled
Copy link
Member

@ITAYC0HEN ITAYC0HEN Aug 7, 2020

Choose a reason for hiding this comment

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

What is "do refresh"? what actually happen? what is refreshed

Copy link
Member Author

@NirmalManoj NirmalManoj Aug 7, 2020

Choose a reason for hiding this comment

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

I have modified this line.

* @brief Decompile the function that contains the specified offset.
*
* @param addr Specified offset/offset in sync.
*/
void doRefresh(RVA addr = Core()->getOffset());
Copy link
Member

@ITAYC0HEN ITAYC0HEN Aug 7, 2020

Choose a reason for hiding this comment

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

Probably a bad naming. I'd expect "decompile" or something similar that actually tells that this function will do decompilation

Copy link
Member Author

@NirmalManoj NirmalManoj Aug 7, 2020

Choose a reason for hiding this comment

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

@ITAYC0HEN In this case, I think "doRefresh" is better as it is actually refreshing the decompiler. I have updated this function with a detailed explanation of what it does.

void setupFonts();
/**
* @brief Update highlights in the text widget
Copy link
Member

@ITAYC0HEN ITAYC0HEN Aug 7, 2020

Choose a reason for hiding this comment

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

what are "highlights"?

Copy link
Member Author

@NirmalManoj NirmalManoj Aug 7, 2020

Choose a reason for hiding this comment

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

I have updated docs explaining what this does.

src/menus/DecompilerContextMenu.h Outdated Show resolved Hide resolved
@NirmalManoj
Copy link
Member Author

NirmalManoj commented Aug 7, 2020

I'd change thigns like "is a reference" to something which makes it clear what "a reference" here. You did it once, when you detailed the things which are considered a reference like "function name, global variable, ....".

@ITAYC0HEN What exactly requires a change here? Are you asking me to modify places that mention "reference (function .." or find keywords that need more documentation? Please give an example.

Copy link
Member

@ITAYC0HEN ITAYC0HEN left a comment

thanks! :)

@NirmalManoj NirmalManoj requested a review from karliss Aug 8, 2020
karliss
karliss approved these changes Aug 8, 2020
@NirmalManoj
Copy link
Member Author

NirmalManoj commented Aug 14, 2020

Please merge this when the CI is green. I will arrange the commits in the next PR to master.

@NirmalManoj NirmalManoj requested a review from karliss Aug 14, 2020
@ITAYC0HEN ITAYC0HEN merged commit 763f3f5 into rizinorg:decompiler-refactoring Aug 14, 2020
9 checks passed
NirmalManoj added a commit that referenced this pull request Aug 14, 2020
NirmalManoj added a commit that referenced this pull request Aug 14, 2020
NirmalManoj added a commit that referenced this pull request Aug 14, 2020
NirmalManoj added a commit that referenced this pull request Aug 14, 2020
NirmalManoj added a commit that referenced this pull request Aug 15, 2020
NirmalManoj added a commit that referenced this pull request Aug 15, 2020
NirmalManoj added a commit that referenced this pull request Aug 15, 2020
@NirmalManoj NirmalManoj moved this from In progress to Done in Improving Decompiler Widget (GSoC) Aug 15, 2020
NirmalManoj added a commit that referenced this pull request Aug 17, 2020
NirmalManoj added a commit that referenced this pull request Aug 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Decompiler Issues and feature requests related to the decompiler in Cutter
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants