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

API for checking if an annotation is a reference or function variable. #17386

Merged

Conversation

NirmalManoj
Copy link
Contributor

@NirmalManoj NirmalManoj commented Jul 31, 2020

Your checklist for this pull request

  • I've read the guidelines for contributing to this repository
  • I made sure to follow the project's coding style
  • I've added tests that prove my fix is effective or that my feature works (if possible)
  • I've updated the documentation and the radare2 book with the relevant information (if needed)

Detailed description

This PR implements the following two functions. Read the documentation below.

/**
 * is_annotation_reference() - Checks if the specified annotation is a reference.
 * @annotation: Pointer to an annotation.
 * 
 * This function recognizes the type of the specified annotation and returns true if its
 * type is any of the following three: R_CODE_ANNOTATION_TYPE_GLOBAL_VARIABLE,
 * R_CODE_ANNOTATION_TYPE_CONSTANT_VARIABLE, R_CODE_ANNOTATION_TYPE_FUNCTION_NAME
 * 
 * Return: Returns true if the specified annotation is a reference.
 */
R_API bool is_annotation_reference(RCodeAnnotation *annotation);
/**
 * is_annotation_variable() - Checks if the specified annotation is a function variable.
 * @annotation: Pointer to an annotation.
 * 
 * This function recognizes the type of the specified annotation and returns true if its
 * type is any of the following two: R_CODE_ANNOTATION_TYPE_LOCAL_VARIABLE,
 * R_CODE_ANNOTATION_TYPE_FUNCTION_PARAMETER
 * 
 * Return: Returns true if the specified annotation is a function variable.
 */
R_API bool is_annotation_variable(RCodeAnnotation *annotation);

...

Test plan

  1. Check code.
  2. Fetch and compile Cutter PR #2352 after compiling this PR. That PR uses this API. So you can check if it's working correctly or not.

...

Closing issues

...

@NirmalManoj NirmalManoj requested review from ITAYC0HEN and karliss Jul 31, 2020
@NirmalManoj NirmalManoj added the API New API requests, changes, removal label Jul 31, 2020
@NirmalManoj NirmalManoj self-assigned this Jul 31, 2020
* frees memory that is dynamically allocated for it.
*
* Return: Nothing.
*/
R_API void r_annotation_free(void *e, void *user);
/**
Copy link
Contributor

@karliss karliss Jul 31, 2020

Choose a reason for hiding this comment

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

Please don't invent your own documentation comment style. If you use doxygen style comments do so consistently. They are not only for human reading but also consumption by tools. See the documentation https://www.doxygen.nl/manual/index.html

Copy link
Contributor Author

@NirmalManoj NirmalManoj Jul 31, 2020

Choose a reason for hiding this comment

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

Radare2 uses this style https://www.kernel.org/doc/html/v4.16/doc-guide/kernel-doc.html#how-to-format-kernel-doc-comments
I have been told to use this and all functions in r_annotated_code.h are documented using this style.

Copy link
Contributor Author

@NirmalManoj NirmalManoj Jul 31, 2020

Choose a reason for hiding this comment

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

Copy link
Contributor

@karliss karliss Jul 31, 2020

Choose a reason for hiding this comment

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

Is it written anywhere r2 documentation that it uses this style? If it I am fine with that, but I haven't seen it used anywhere except r_annotated_code.

Copy link
Contributor

@karliss karliss Jul 31, 2020

Choose a reason for hiding this comment

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

This https://github.com/radareorg/radare2/blob/master/DEVELOPERS.md claims that r2 uses doxygen C-style comments.

Copy link
Contributor Author

@NirmalManoj NirmalManoj Jul 31, 2020

Choose a reason for hiding this comment

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

@ITAYC0HEN @thestr4ng3r I'm not sure when this was decided. See this comment by @ITAYC0HEN #16939 (review) It was discussed when I was working on #16939 .

Copy link
Contributor

@XVilka XVilka Aug 3, 2020

Choose a reason for hiding this comment

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

Doxygen is the way to go.

@@ -23,6 +23,14 @@ R_API void r_annotation_free(void *e, void *user) {
}
}

R_API bool is_annotation_reference(RCodeAnnotation *annotation) {
Copy link
Contributor

@karliss karliss Aug 3, 2020

Choose a reason for hiding this comment

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

Please consider renaming these functions. Most R_API functions use common prefix, this helps avoiding naming conflict and improves discoverability. Maybe r_annotation_is_reference and r_annotation_is_variable.

Copy link
Contributor Author

@NirmalManoj NirmalManoj Aug 3, 2020

Choose a reason for hiding this comment

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

I will rename them.

Copy link
Contributor Author

@NirmalManoj NirmalManoj Aug 3, 2020

Choose a reason for hiding this comment

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

@karliss I have renamed functions. If you are fine with the code now, please merge it. I think PR #2352 Cutter can get merged tonight. Since it uses the new API implemented in this PR, I want this to get merged first.

I will send a separate PR with documentation using Doxygen tonight, but I think it can be another PR as it could take more time to review.

R_API bool r_annotation_is_reference(RCodeAnnotation *annotation);
/**
* r_annotation_is_variable() - Checks if the specified annotation is a function variable.
* @annotation: Pointer to an annotation.
Copy link
Contributor

@ITAYC0HEN ITAYC0HEN Aug 3, 2020

Choose a reason for hiding this comment

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

shouldn't it be @param annotation ?

Copy link
Contributor Author

@NirmalManoj NirmalManoj Aug 3, 2020

Choose a reason for hiding this comment

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

*/
R_API bool r_annotation_is_reference(RCodeAnnotation *annotation);
/**
* r_annotation_is_variable() - Checks if the specified annotation is a function variable.
Copy link
Contributor

@ITAYC0HEN ITAYC0HEN Aug 3, 2020

Choose a reason for hiding this comment

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

Should you include the name of the function in the docs? "r_annotation_is_variable() "? Also, if you include this, the signature is wrong as it receive parameters

Copy link
Contributor Author

@NirmalManoj NirmalManoj Aug 3, 2020

Choose a reason for hiding this comment

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

This is how it is in Kernel-doc, if I understand correctly. As I said, I'm going to update this to Doxygen, in another PR. So let's not worry about documentation for now.

Copy link
Contributor

@ITAYC0HEN ITAYC0HEN left a comment

Please make sure to fix the docs in the upcoming pr.

@ITAYC0HEN ITAYC0HEN merged commit 2248961 into radareorg:decompiler-refactoring Aug 3, 2020
14 checks passed
@NirmalManoj
Copy link
Contributor Author

NirmalManoj commented Aug 3, 2020

Here's the new PR fixing the documentation: #17397

@NirmalManoj NirmalManoj moved this from In progress to Done in Improving Decompiler Widget (GSoC) Aug 3, 2020
NirmalManoj added a commit to NirmalManoj/radare2 that referenced this pull request Aug 6, 2020
ITAYC0HEN pushed a commit that referenced this pull request Aug 10, 2020
…edCode (#17429)

* Annotation for function name (#17204)
* Annotations for Constant Variables and Global Variables for the decompiler (#17281)
* Annotation For Function Variables (#17375)
* function variable annotation added (includes local variable and function parameter)
* API for checking if an annotation is a reference or function variable. (#17386)
* Update docs in annotate code API  (#17397)
* Unit tests for annotated code API (#17403)
ret2libc pushed a commit to ret2libc/radare2 that referenced this pull request Sep 1, 2020
…edCode (radareorg#17429)

* Annotation for function name (radareorg#17204)
* Annotations for Constant Variables and Global Variables for the decompiler (radareorg#17281)
* Annotation For Function Variables (radareorg#17375)
* function variable annotation added (includes local variable and function parameter)
* API for checking if an annotation is a reference or function variable. (radareorg#17386)
* Update docs in annotate code API  (radareorg#17397)
* Unit tests for annotated code API (radareorg#17403)
ret2libc pushed a commit that referenced this pull request Sep 2, 2020
…edCode (#17429)

* Annotation for function name (#17204)
* Annotations for Constant Variables and Global Variables for the decompiler (#17281)
* Annotation For Function Variables (#17375)
* function variable annotation added (includes local variable and function parameter)
* API for checking if an annotation is a reference or function variable. (#17386)
* Update docs in annotate code API  (#17397)
* Unit tests for annotated code API (#17403)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API New API requests, changes, removal
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

4 participants