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

C API documentation #2959

Merged
merged 8 commits into from Sep 21, 2015
Merged

C API documentation #2959

merged 8 commits into from Sep 21, 2015

Conversation

sagmor
Copy link
Contributor

@sagmor sagmor commented Sep 21, 2015

I started to work on documenting MRuby's C API.

While working on a gem project, I started to get locked quite often on "whats the function that does x?" moments so I started to document the methods I found on the process.

I noticed there are some methods already documented at doc/api but I believe it's better to have the API documented on the headers, so I've been taking those docs, and merging them into the headers as well.

I'm leaving this pull request to get some feedback before putting more effort into it.

Some of the things I've experimented with related with documenting MRuby's C API are:

  • Generate a simple API website: the generated site is available right now at http://sagmor.com/mruby-c-api/ and its source at https://github.com/sagmor/mruby-c-api. eventually with a "c-api" repo it could be automated and live in mruby.org/c-api for example.
  • Grabbed some macros like MRB_API, mrb_deprecated and moved them to mruby/common.h
  • Add a couple macros MRB_BEGIN_DECL and MRB_END_DECL to get rid of those ugly extern "C" all over the headers.
  • Replacing some #define aliases with inline function, this is just an experiment/proposal, they look more consistent while documenting (see mrb_class_new_instance)
  • typedeffed mrb_get_args format string to document it separately (See mrb_args_format

As I said, I'm leaving this here to see what you think of these approaches before putting more effort into them.

@matz
Copy link
Member

matz commented Sep 21, 2015

Looks good. But this pull-request has several conceptual changes in one. Next time, could you separate pull-requests for each concept (and gain more contribution measurement)?

matz added a commit that referenced this pull request Sep 21, 2015
@matz matz merged commit e47f0fc into mruby:master Sep 21, 2015
@bovi
Copy link
Member

bovi commented Sep 22, 2015

Hi @sagmor,

this is great work. I did some time ago work on the mrbdoc tool and we integrated it into the official website (https://github.com/mruby/mruby.github.io/blob/master/gen_mrbdoc.rb). Your generated stuff looks much better and I would encourage you (of course only if you have time and fun) to consider a pull request to exchange the mrbdoc integration in the mruby.github.io repo [https://github.com/mruby/mruby.github.io] with your variant. In this way we can keep the documentation via your tool up-to-date on the official website.

@sagmor
Copy link
Contributor Author

sagmor commented Sep 22, 2015

@matz sorry for the "several conceptual changes in one" but things came out as I was playing with the headers. Glad you merged it though.

@bovi, That was my idea. I'm pushing the generation script into mruby.org repo right now.

@cremno
Copy link
Contributor

cremno commented Sep 27, 2015

Why do we need MRB_INLINE?

@takahashim
Copy link
Contributor

I guess that:

  • Macro has no type information (without preprocessor), so use function declarations
  • Function call has overhead, so use inline functions
  • Inline definition has compatibility issue, so define and use MRB_INLINE

@furunkel
Copy link
Contributor

Should probably be named mrb_inline for consistency, though ?

@sagmor
Copy link
Contributor Author

sagmor commented Sep 28, 2015

The problem is that MRB_API (extern) is mutually exclusive with MRB_INLINE (inline)
I tried to place them together and failed miserably 😛

CC    src/array.c -> build/host/src/array.o
In file included from /Users/sagmor/Development/mruby/mruby/src/array.c:7:
/Users/sagmor/Development/mruby/mruby/include/mruby.h:288:9: error: cannot combine with previous 'extern' declaration specifier
MRB_API MRB_INLINE mrb_value mrb_class_new_instance(mrb_state *mrb, mrb_int argc, const mrb_value *argv, struct RClass *c)
        ^
/Users/sagmor/Development/mruby/mruby/include/mruby/common.h:51:21: note: expanded from macro 'MRB_INLINE'
# define MRB_INLINE static inline
                    ^
1 error generated.
In file included from /Users/sagmor/Development/mruby/mruby/src/array.c:7:
/Users/sagmor/Development/mruby/mruby/include/mruby.h:288:9: error: cannot combine with previous 'extern' declaration specifier
MRB_API MRB_INLINE mrb_value mrb_class_new_instance(mrb_state *mrb, mrb_int argc, const mrb_value *argv, struct RClass *c)
        ^
/Users/sagmor/Development/mruby/mruby/include/mruby/common.h:51:21: note: expanded from macro 'MRB_INLINE'
# define MRB_INLINE static inline
                    ^
1 error generated.
rake aborted!

@sagmor
Copy link
Contributor Author

sagmor commented Sep 28, 2015

As @takahashim says, inline functions keeps the fact that It's a function without masking it under a "constant" that makes a lot easier to document C Headers consistently (all while keeping the performance of the #define).

Right now I'm playing with that fact to further improve the docs with a yard plugin it's a bit early yet but here's a demo of it's current state when executed on MRuby's core

@cremno
Copy link
Contributor

cremno commented Sep 28, 2015

The problem is that MRB_API (extern) is mutually exclusive with MRB_INLINE (inline)

extern and static are mutually exclusive. extern and inline aren't.

Let me rephrase my question: what's the rationale behind the introduction of MRB_INLINE? Just consistency with MRB_API? Or why is directly writing static inline not acceptable?

Right now I'm playing with that fact to further improve the docs with a yard plugin it's a bit early yet but here's a demo of it's current state when executed on MRuby's core

That's great! (I like yard.)

@sagmor
Copy link
Contributor Author

sagmor commented Sep 29, 2015

@cremno Actually I took the inspiration from libgit2 and to be honest MRB_INLINE was a suggestion to turn defines into actual functions for API consistency but it's open for debate if anyone has a better idea 👍

I will make a few experiments defining mrb_inline as just inline and see how it plays along and see, not sure how the static, extern and inline keywords behave together.

And by the way, The docs are getting better (See: sagmor.com/mruby)

@cremno
Copy link
Contributor

cremno commented Sep 30, 2015

http://www.greenend.org.uk/rjk/tech/inline.html is a good explanation.

I think the linked libgit2 code is a bit too cautious. I don't know its history though. Maybe there was a reason to define it that way.

However mruby already provides inline (as macro which expands to __inline) for older MSVC versions. So in my opinion MRB_INLINE doesn't improve or solve anything. Would it be okay to remove it and use static inline again instead?

@tsahara tsahara mentioned this pull request Oct 1, 2015
36 tasks
@sagmor
Copy link
Contributor Author

sagmor commented Oct 1, 2015

@cremno Further investigating I found out there's an __attribute__((always_inline)) that

According to the GCC User Manual one can force a function to be inlined using the always_inline attribute

But I don't have enough info to make an informed choice.
Now from what I see these are the options we have:

1:
#define mrb_foo /* code */
/*
 * What we have now, fast and easy.
 *  but in a way inconsistent (for documentation purposes)
 */

2:
#define MRB_INLINE static inline __attribute__((always_inline))
MRB_INLINE void mrb_foo( void ) { /* code */ }
/*
 * We turn the define into a function and get more context for the documentation
 */

3:
static inline void mrb_foo( void ) { /* code */ }
/*
 * Pretty much the same as above.
 * Not sure if the  __attribute__ has a real impact, and the need of a 
 * MRB_INLINE define seems like a keyword to keep it visually consistent with MRB_API
 */

4:
#define mrb_inline inline __attribute__((always_inline))
MRB_API mrb_inline void mrb_foo( void ); /* and code goes on some .c file */
/*
 * I like how this looks.
 * mrb_inline is consistent with other "flags" like mrb_deprecated or mrb_noreturn
 * but it forces the function's code to live on a different .c file. Which means more
 * work but also means the implementation get's hidden/encapsulated
 */

From how it looks, I like the fourth option but I have no idea if there's a real performance difference on each option and how far we should move to replace those defines

@sagmor sagmor mentioned this pull request Oct 8, 2015
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.

None yet

6 participants