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

fix https://github.com/ruby/ruby/pull/1722 #4815

Merged
merged 162 commits into from
Sep 10, 2021
Merged

Conversation

shyouhei
Copy link
Member

@shyouhei shyouhei commented Sep 8, 2021

... by actually documenting everything.

I'm pretty sure I made literally thousands of mistakes here (this is a 25k+ LOC pull request), but I believe this is at least better than nothing.

Truly editorial fix for comments.  This works better with Emacs'
set-justification-full function. [ci skip]
This script is called from Doxygen many times.  Worth optimising.
[ci skip]
Didn't question the current settings.  This changeset just re-applied
`doxygen -g` against:

doxygen 1.9.0 (1e72202d8fa0e9d2b3f2a29c88ec4f5790a0a4e2)

[ci skip]
Let our VCS manage old contents. [ci skip]
`make capi` warned:

> warning: Included by graph for 'dllexport.h' not generated, too many nodes (85)

[ci skip]
The new Doxyfile.tmpl says:

> # Values that contain spaces should be placed between quotes (\" \").

[ci skip]
It is easier to maintain (e.g. sort them). [ci skip]
This enables me to write `@shyouhei` in C comments without complained by
doxygen that @shyouhei is an unknown special command. [ci skip]
These contents are purely implementation details, not worth appearing in
CAPI documents. [ci skip]
Must not be a bad idea to improve documents. [ci skip]
Must not be a bad idea to improve documents. [ci skip]
Must not be a bad idea to improve documents. [ci skip]
 Must not be a bad idea to improve documents. [ci skip]
Must not be a bad idea to improve documents. [ci skip]
Must not be a bad idea to improve documents. [ci skip]
Unlike other "add doxygen" commits this one adds a preprocessor branch
that doxygen would process.  This prevents it from parsing other parts
of the file.
Must not be a bad idea to improve documents. [ci skip]
Must not be a bad idea to improve documents. [ci skip]
Must not be a bad idea to improve documents. [ci skip]
Must not be a bad idea to improve documents. [ci skip]
Must not be a bad idea to improve documents. [ci skip]

In fact many functions declared in the header file are already
documented more or less.  They were just copy & pasted, with applying
some style updates.
Must not be a bad idea to improve documents. [ci skip]

In fact many functions declared in the header file are already
documented more or less.  They were just copy & pasted, with applying
some style updates.
Must not be a bad idea to improve documents. [ci skip]
Must not be a bad idea to improve documents. [ci skip]
Must not be a bad idea to improve documents. [ci skip]

In fact many functions declared in the header file are already
documented more or less.  They were just copy & pasted, with applying
some style updates.
Must not be a bad idea to improve documents. [ci skip]
Must not be a bad idea to improve documents. [ci skip]
Must not be a bad idea to improve documents. [ci skip]
Must not be a bad idea to improve documents.
@shyouhei shyouhei requested a review from sonots September 8, 2021 08:16
@ioquatix
Copy link
Member

ioquatix commented Sep 8, 2021

This looks great.

Did you automate the process or do it by hand?

I have some bindings for ffi-clang which allow you to extract comments from the C/C++ AST. Maybe helpful for mechanical transforms.

@shyouhei
Copy link
Member Author

shyouhei commented Sep 9, 2021

I did this by hand simply because I wasn't aware of your ffi-clang.

Silly bug, they write back consumed bytes through passed pointers.  Must
never be pure functions.

ruby_scan_oct does not refer any static variables so it can still be
__declspec(noalias), while ruby_scan_hex is not because it reads from
ruby_digit36_to_number_table.
@ioquatix
Copy link
Member

ioquatix commented Sep 9, 2021

I'm not suggesting that ffi-clang would easily solve your problem (I'm not sure about how easy AST rewriting is with comments), but parsing symbols with attached comments is relatively straight forward. If you think it can help, I'm happy to revive the gem and update it to the latest clang.

https://github.com/ioquatix/ffi-clang

Example: https://github.com/ioquatix/ffi-clang/blob/master/examples/docs.rb

I was not aware of this because I use clang these days.
Explicit check done at runtime.
I was not aware of this because I use clang these days.
This particular NULL check must be a good thing to do both statically
and dynamically.
Noticed that defs/gmake.mk has `exts: rubyspec-capiext` dependency only
when $ENABLE_SHARED is true.  This one adds extra tests so we basically
welcome.  Why not default it on.
These warnings are okay here.
RBIMPL_WARNING_PUSH is a 3.0 feature.  Rubyspec OTOH has to support 2.x.
LTO is about static links.  Makes no sense to have DLLs.
@shyouhei
Copy link
Member Author

shyouhei commented Sep 9, 2021

CI is green... Let's merge this.

@shyouhei shyouhei merged commit cb4e2cb into ruby:master Sep 10, 2021
@larskanis
Copy link
Contributor

@shyouhei Wow, this is a great improvement! I've spend so many hours to inspect ruby sources in order to understand the C-API for writing extensions. This work makes it a lot easier now.

doc/extension.rdoc describes many functions in one sentence. This is now somewhat duplicated in the header files. It would be interesting to link them together.

Is there an official URL for the generated Doxygen pages?

@shyouhei
Copy link
Member Author

@larskanis extension.rdoc has its own purpose (provides basics rather than comprehensive lists of things). I don't think we can replace that document using doxygen.

But yes, we lack official doxygen pages, which are definitely nice if any.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
4 participants