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

Add Doxygen \memberof annotations #3027

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

amomchilov
Copy link
Contributor

@amomchilov amomchilov commented Aug 30, 2024

Doxygen's \memberof command lets you associate C functions to types, to make them render as if they were actual methods. This is in a similar spirit to \extends, which we use to simulate inheritance from pm_node.

Demo:

image

It's possible to hide the PRISM_EXPORTED_FUNCTION macro spam from these return types, but I'll do that in a separate PR.

@amomchilov amomchilov force-pushed the amomchilov/memberof branch 2 times, most recently from 529652b to ba6d3b5 Compare August 30, 2024 21:13
Copy link
Collaborator

@kddnewton kddnewton left a comment

Choose a reason for hiding this comment

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

A couple comments. Also it seems like sometimes we have memberof pm_*_t and sometimes we have memberof pm_*. I think I would prefer to use the typedef.

include/prism.h Outdated
@@ -43,6 +43,7 @@
* The prism version and the serialization format.
*
* @returns The prism version as a constant string.
* \public \memberof pm_parser
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would prefer that this not be a member of parser, since it does not accept it as a parameter/doesn't correspond to a parser.

*/
PRISM_EXPORTED_FUNCTION pm_node_t * pm_parse(pm_parser_t *parser);

/**
* This function is used in pm_parse_stream to retrieve a line of input from a
* This function is used in pm_parse_stream() to retrieve a line of input from a
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
* This function is used in pm_parse_stream() to retrieve a line of input from a
* This function is used in pm_parse_stream to retrieve a line of input from a

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The () makes this into link to the pm_parse_stream() function that uses this callback.

* stream. It closely mirrors that of fgets so that fgets can be used as the
* default implementation.
* \public \memberof pm_parser
Copy link
Collaborator

Choose a reason for hiding this comment

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

This shouldn't be marked as a member of parser, as it's a user-defined callback that does not correspond to a parser.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's still strongly related to the parser (similar to a type alias or nested interface defined in a Ruby class), and renders nicely on the same page under "Public types":

image

WDYT?

include/prism.h Outdated Show resolved Hide resolved
include/prism/node.h Outdated Show resolved Hide resolved
@@ -112,6 +118,7 @@ PRISM_EXPORTED_FUNCTION const char * pm_node_type_to_str(pm_node_type_t node_typ
* @param node The root node to start visiting from.
* @param visitor The callback to call for each node in the subtree.
* @param data An opaque pointer that is passed to the visitor callback.
* \public \memberof pm_node
Copy link
Collaborator

Choose a reason for hiding this comment

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

This one I disagree with, as it's more related to the visitor (the function callback). So I would exclude this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What do you think about switching it to "related"?

It'll still help discoverability, since someone looking at this API would probably look in pm_node to start

@@ -123,6 +130,7 @@ PRISM_EXPORTED_FUNCTION void pm_visit_node(const pm_node_t *node, bool (*visitor
* @param node The node to visit the children of.
* @param visitor The callback to call for each child node.
* @param data An opaque pointer that is passed to the visitor callback.
* \public \memberof pm_node
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same here, this is more related to the visitor, so I don't think this should be associated with a particular node.

include/prism/regexp.h Outdated Show resolved Hide resolved
include/prism/regexp.h Outdated Show resolved Hide resolved
include/prism/regexp.h Outdated Show resolved Hide resolved
@amomchilov amomchilov force-pushed the amomchilov/memberof branch 2 times, most recently from 6f947bd to 3f77637 Compare September 16, 2024 13:12
@amomchilov
Copy link
Contributor Author

@kddnewton Ready for re-review

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.

2 participants