RetroArch icon indicating copy to clipboard operation
RetroArch copied to clipboard

Add Doxygen-styled comments to parts of the libretro API

Open JesseTG opened this issue 2 years ago • 18 comments

Description

This pull request will add Doxygen-styled comments to various parts of the libretro API to simplify their use. Even if HTML pages are not generated or hosted, Doxygen is common enough that IDEs will recognize it. Here's what it looks like in CLion:

Screenshot 2023-08-23 075155

The comments are written to adhere to SemBr for easier diffs and clearer sentence structure. Essentially, line breaks are treated like punctuation.

This PR will not:

  • Violate C90. All additions come as /** standard C90 comments */.
  • Change how types, functions, macros, etc. are declared.
  • Generate documentation pages. This just adds comments.
  • Document RetroArch internals. This is for the benefit of libretro-common.

This PR, as of this writing, is only a sample. If @LibretroAdmin approves the general style, then I will proceed to document the rest of <libretro.h> (plus some other libretro-common APIs I've used) in the same format.

Reviewers

@LibretroAdmin @RobLoach

Status

The goal of this PR is to document ~all public-facing~ the parts of libretro-common that I've used. Here's where that stands:

JesseTG avatar Aug 23 '23 11:08 JesseTG

I've been wanting doxygen in here FOREVER. Happy to help out on its transition, if the rest of the libretro team are :+1:

RobLoach avatar Aug 24 '23 16:08 RobLoach

I've been wanting doxygen in here FOREVER. Happy to help out on its transition, if the rest of the libretro team are 👍

Great! How do you wanna divvy up the work? I'm happy to flesh out the docs for the environment calls plus any APIs that I've used so far. (That includes the VFS, task queue, and image scaler.) I'm also happy to proofread for spelling and clarity.

The APIs I've contributed already use Doxygen comments.

JesseTG avatar Aug 24 '23 17:08 JesseTG

Oh, hey, it looks like there's a (default) Doxyfile in the repo already. I'll have to fine-tune it at some point.

JesseTG avatar Sep 07 '23 01:09 JesseTG

I think my vision for this PR's roadmap could use a bit of clarification.

  1. I'd like to get doc comments with correct information written for all public-facing APIs defined in libretro-common.
    • This step also includes refactoring "pseudo-Doxygen" to comments (like there was in clamping.h, for instance)
    • For compatibility headers that fill in for missing C functions, not every symbol has to be documented. Only those that have libretro-specific limits like getopt need their own body copy. Otherwise, I think liberal use of @see tags (plus the occasional file-level doc comment) would suffice. For example, I wanna link to this man page for strlcpy somewhere in <compat/strl.h>. Ditto for the OpenGL headers in glsym.
  2. Once all the info is written and fact-checked, we can make a decision about how we actually expect to publish documentation, if at all. Remember, these doc comments will very likely be shown inline by IDEs; they will be useful even if we never run Doxygen.
  3. Once we make that decision, we can tweak the Doxyfile and doc comment tags (groups, examples, etc.) to be in line with it.
  4. After that happens, this PR will be good to merge in my mind. I don't want this to languish for too long, as there are other things I'd like to work on.
    • Example: in the similar spirit of annotating declarations to make the APIs easier to use, I want to slap these annotations (or local equivalents) on various symbols. That'll be a later PR, though.

JesseTG avatar Sep 09 '23 17:09 JesseTG

I've added a list of all the files that need to be documented. I would greatly appreciate some help checking these boxes off.

JesseTG avatar Sep 25 '23 14:09 JesseTG

@RobLoach I had documentation I'd just written for RETRO_DEVICE that I hadn't committed yet. Maybe we should coordinate what exactly we're working on.

I have docs for network_stream.h, retro_endianness.h, retro_miscellaneous.h, file_path.h, file_stream.h, features_cpu.h, and strl.h in the oven. I will finish these files before I start any new ones. What about you?

JesseTG avatar Oct 03 '23 22:10 JesseTG

Oops! Feel free to revert or force push your work 😉

RobLoach avatar Oct 03 '23 22:10 RobLoach

Oops! Feel free to revert or force push your work 😉

It's fine, I can just merge them; we each covered some ground that the other didn't. That said, I still think we should coordinate which headers we're working on. I've given my near-term plans, what about yours?

JesseTG avatar Oct 03 '23 22:10 JesseTG

I have just been touching up when using the API elsewhere. But could help with audio_mix stuff.

I can be sure to send PRs so that there aren't any conflict.

RobLoach avatar Oct 03 '23 23:10 RobLoach

Sounds good. You could send them against either this branch on my fork, or just on RetroArch directly. I don't have a strong preference.

JesseTG avatar Oct 03 '23 23:10 JesseTG

In the interest of getting this out the door, I'm going to reduce the scope of this PR to libretro.h and headers I've used.

JesseTG avatar Nov 02 '23 01:11 JesseTG

That seems appropriate. We could take on the rest separately.

RobLoach avatar Nov 02 '23 03:11 RobLoach

That seems appropriate. We could take on the rest separately.

Sounds good. At some point after this is merged, we could take a look at integrating the contents into the docs repo. I'm thinking that repo's workflow could do the following, in order:

  1. Clone the latest commit of RetroArch.
  2. Generate doc pages with MkDoxy or similar, consistent with the style of the rest of the doc pages.

JesseTG avatar Nov 02 '23 16:11 JesseTG

I'm still working on this. I've been prioritizing melonDS DS, but I've been squeezing in some work here whenever I'm blocked on something. I've finished with most parts of libretro.h. Once I complete it (and one other header whose changes I haven't pushed yet), I'll go around requesting reviews.

JesseTG avatar Dec 27 '23 16:12 JesseTG

In the interest of not letting this languish while I move on to something else, I've decided to call this branch done. Any gaps in it will be filled in future PRs. In the meantime, could I get some eyes on the contents?

JesseTG avatar Jan 03 '24 17:01 JesseTG

@LibretroAdmin I believe this is good to go. Had another look through and there isn't any API or function definition that is changing, it's all documentation.

One exception; I do add a new enum here for RETRO_ENVIRONMENT_GET_AUDIO_VIDEO_ENABLE. However, the size and range of the enum's values are consistent with the hardcoded numbers in the original documentation, so I don't foresee any ABI problems.

JesseTG avatar Jan 05 '24 05:01 JesseTG

@Jamiras can still review it.

LibretroAdmin avatar Feb 23 '24 18:02 LibretroAdmin

Sounds good with me. The more eyes we have on this the better. When we decide to merge this forwards, it may be best to "Squash and merge" so that the git history doesn't get too crazy. Screenshot from 2024-02-23 13-56-40

RobLoach avatar Feb 23 '24 18:02 RobLoach