Add RRGraph-related API to documentation and clean-up RRSpatialLookup methods
#1837
Conversation
…sible in docxygen
|
@vaughnbetz @hzeller This pull request is ready for your review. It does not change codes. so I expect the CI to be green. Since this is the first time I use doxygen in documentation, any comments on beautifying the code comments are welcomed. For @hzeller 's comments on removing RRSpaitial constructors, I will address it in another pull request. My intention is to narrow the scope of this PR in documentation changes only. Since I have exploited readthedocs in previewing doc changes. The commit number is large. You may squash and merge. |
There was a problem hiding this comment.
Looks good.
- It seems the RRGraphView comments aren't in the API though -- can you also add Doxygen comments to it, or should Ethan do that? If Ethan, we should make sure he has an issue.
- I think the TODO comments are for API developers rather than users, and hence should be regular (not Doxygen) comments. What do you think?
vpr/src/device/rr_graph_builder.h
Outdated
| * | ||
| * Note: | ||
| * @note | ||
| * - This is the only data structre allowed to modify a routing resource graph |
| * - track index in a routing channel when type is CHANX/CHANY | ||
| * @param side is the side of node on the tile, applicable to OPIN/IPIN | ||
| * | ||
| * @note a node added with this call will not create a node in the rr_graph_storage node list | ||
| * You MUST add the node in the rr_graph_storage so that the node is valid | ||
| * | ||
| * TODO: Consider to try to return a reference to *this so that we can do chain calls |
There was a problem hiding this comment.
Maybe the TODO comment should be outside the doxygen commenting (put as just a regular comment) so it doesn't show up in the API comment?
| * | ||
| * Note: currently this function only accepts SOURCE/SINK nodes. May unlock for the other types | ||
| * @note currently this function only accepts SOURCE/SINK nodes. May unlock for the other types | ||
| * depending on needs | ||
| * | ||
| * TODO: Consider to make a high-level API to duplicate the nodes for large blocks. |
There was a problem hiding this comment.
Maybe the TODO comment should be outside the doxygen comments so it is a note to developers rather than API users?
|
@ethanroj23 : see the comments above on RRGraphView Doxygen commenting. |
@vaughnbetz I can definitely add Doxygen comments to the RRGraphView files. Currently. my concern is that my modification will cause merging conflicts with Ethan's pull requests. I can do follow-up pull request during the gap of his pull requests or work with him on reworking the comments. For the typo fix and TODO comments, they make sense to me. I will address the comment in this pull request. |
…m Doxygen comments
|
@vaughnbetz I have addressed the comments. I also double checked the documentation and see that the TODO comments are not shown in the VPR API pages. Regarding the RRGraphView comments, please let me know what you think. I will take action then. |
|
Thanks @tangxifan . @ethanroj23 : I think it's best if you make the RRGraphView comments doxygen compatible to avoid merge conflicts (and to group related work together) as @tangxifan suggests. |
Description
This is a follow-up pull request to address the comments in #1828
It also addressed the issue #1834
Checklist:
RRGraph-related API to documentationrr_graph_builder.hand ``rr_spatial_lookup.h" to be doxgen compatible.RRSpatialLookup()constructor**Note: the code comments in
rr_graph_view.his untouched to avoid merging conflicts as @ethanroj23 is working on it.Related Issue
This pull request is part of the refactoring effort of routing resource graph #1828
Motivation and Context
Along with refactoring, RRGraph APIs are being developed with enriched comments. It is time to build the documentation for the APIs.
How Has This Been Tested?
The documentation changes can be previewed at the readthedocs in the pull request checks.
Screenshots taken from the readthedocs:
Types of changes
Checklist: