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.

Sign up for GitHub

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

Jump to bottom

Update docs in annotate code API #17397

Merged
merged 11 commits into from Aug 5, 2020
Merged

Update docs in annotate code API #17397

merged 11 commits into from Aug 5, 2020

Conversation

NirmalManoj
Copy link
Contributor

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
Earlier, I had used kernel-docs documentation style for documenting new API functions created for the annotations in the decompiler. This PR replaces these documentations by Doxygen.

...

Test plan

  1. Make sure this compiles properly.
  2. Read the documentation and make sure nothing is wrong.

...

Closing issues

...

@NirmalManoj NirmalManoj self-assigned this Aug 3, 2020
@NirmalManoj NirmalManoj mentioned this pull request Aug 3, 2020
Merged
4 tasks
karliss
karliss suggested changes Aug 3, 2020
View reviewed changes
Copy link
Contributor

@karliss karliss left a comment

Choose a reason for hiding this comment

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

Comment for RCodeAnnotationType isn't up to date anymore. The first sentence provides zero information.
There are now more than two types. It would probably be more suitable to document each enum value inline. Here is an exmple for that https://www.doxygen.nl/manual/docblocks.html#memberdoc

libr/include/r_core.h Outdated Show resolved Hide resolved
libr/include/r_util/r_annotated_code.h Outdated Show resolved Hide resolved
libr/include/r_util/r_annotated_code.h Outdated Show resolved Hide resolved
libr/include/r_util/r_annotated_code.h Outdated Show resolved Hide resolved
libr/include/r_util/r_annotated_code.h Outdated Show resolved Hide resolved
karliss
karliss suggested changes Aug 3, 2020
View reviewed changes
Copy link
Contributor

@karliss karliss left a comment

Choose a reason for hiding this comment

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

It would be good to document the inclusiveness or exclusiveness r_code_annotation_t start and end positions.

@github-actions github-actions bot added the API New API requests, changes, removal label Aug 3, 2020
@NirmalManoj
Copy link
Contributor Author

It would probably be more suitable to document each enum value inline.

Done.

@NirmalManoj
Copy link
Contributor Author

It would be good to document the inclusiveness or exclusiveness r_code_annotation_t start and end positions.

Done.

karliss
karliss reviewed Aug 4, 2020
View reviewed changes
libr/include/r_util/r_annotated_code.h Outdated Show resolved Hide resolved
libr/include/r_util/r_annotated_code.h Outdated Show resolved Hide resolved
karliss
karliss suggested changes Aug 4, 2020
View reviewed changes
Copy link
Contributor

@karliss karliss left a comment

Choose a reason for hiding this comment

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

As a general advice if all the words in comment are the same as declaration it means that documentation doesn't provide reader with any information and it was waste of time for both person writing it and person reading it.
Don't write "documentation" for the sake of writing it, write it to provide useful information to the caller of function about how to use it correctly and what can it be used for. Avoid repeating things which are already expressed in the naming of function and the type system.

Some bad examples:

  • RCodeAnnotationType type; /**< Type of the annotation. */ - who could have guessed that code annnotation type named type is type of the annotation.
  • R_CODE_ANNOTATION_TYPE_FUNCTION_PARAMETER, /*!< Specified range in annotation represents a function parameter. */

libr/include/r_util/r_annotated_code.h Outdated Show resolved Hide resolved
ITAYC0HEN
ITAYC0HEN approved these changes Aug 5, 2020
View reviewed changes
Copy link
Contributor

@ITAYC0HEN ITAYC0HEN left a comment

Choose a reason for hiding this comment

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

Looks good 👍

@NirmalManoj
Copy link
Contributor Author

NirmalManoj commented Aug 5, 2020 •
edited

Some bad examples:

  • RCodeAnnotationType type; /**< Type of the annotation. */ - who could have guessed that code annnotation type named type is type of the annotation.
  • R_CODE_ANNOTATION_TYPE_FUNCTION_PARAMETER, /*!< Specified range in annotation represents a function parameter. */

@karliss I'm not sure what all are unnecessary explanations. The second one here doesn't seem useless to me. If it is useless, I don't see how any documentation for the enum r_code_annotation_type_t is useful.

@ITAYC0HEN ITAYC0HEN merged commit 87ce9f6 into radareorg:decompiler-refactoring Aug 5, 2020
@karliss
Copy link
Contributor

karliss commented Aug 5, 2020

If it is useless, I don't see how any documentation for the enum r_code_annotation_type_t is useful.

Yes most of the descriptions for r_code_annotation_type_t don't provide any information. I think one or two had unique words providing little bit of additional information.

@karliss karliss moved this from In progress to Done in Improving Decompiler Widget (GSoC) Aug 6, 2020
NirmalManoj added a commit to NirmalManoj/radare2 that referenced this pull request Aug 6, 2020
@NirmalManoj
Update docs in annotate code API (radareorg#17397)
0b1209a
NirmalManoj added a commit that referenced this pull request Aug 9, 2020
@NirmalManoj
Update docs in annotate code API (#17397)
3ab30a5
@NirmalManoj NirmalManoj mentioned this pull request Aug 9, 2020
Merged
3 tasks
ITAYC0HEN pushed a commit that referenced this pull request Aug 10, 2020
@NirmalManoj
Add Function Name, Constants, Globals and Local Variables to RAnnotat…
b4677b4 
…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
@NirmalManoj @ret2libc
Add Function Name, Constants, Globals and Local Variables to RAnnotat…
7bf59de 
…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
@NirmalManoj @ret2libc
Add Function Name, Constants, Globals and Local Variables to RAnnotat…
4ab73aa 
…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
Reviewers

@karliss karliss karliss approved these changes

@ITAYC0HEN ITAYC0HEN ITAYC0HEN approved these changes

@trufae trufae Awaiting requested review from trufae

Assignees

@NirmalManoj NirmalManoj

Labels
API New API requests, changes, removal
Projects
No open projects
Improving Decompiler Widget (GSoC)
  
Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@NirmalManoj @karliss @ITAYC0HEN