gtamba / spice_rub

Ruby wrapper for the NASA-JPL CSPICE library via SciRuby
Other
9 stars 3 forks source link

C identifiers should have prefixes #8

Closed translunar closed 8 years ago

translunar commented 8 years ago

Since you're writing in C and don't have namespaces, you should really prefix your global C functions and identifiers (e.g., SHORT, LONG, EXPLAIN, ALL, unload, etc).

#define SPICE_SHORT 0

for example. You could also call it RUB_SHORT if you want something easier to type.

That way it doesn't cause a namespace collision. (However, if you're using SHORT because it's used this way in the SPICE code, it's fine. Please add a comment if that's the case.)

For stuff like furnsh, you also need a prefix — UNLESS you want to declare the function static. If you make it static, it won't be visible to other sources during compilation; it'll only be available within spice_rub.c.

So, either of these:

static VALUE unload(...)

or

VALUE rub_unload(...)

Make sense?

translunar commented 8 years ago

If you're planning on using C++ as well, please let me know, as there are other options in that case.

gtamba commented 8 years ago

@mohawkjohn :

The Macros were the exact parameters required by the SPICE function getmsg_c so I've renamed them to avoid name collisions. As of now I have no need to use C++, but I plan on introducing nmatrix as a development dependency, will C++ be necessary to use the NMatrix C API?

Is there any crucial drawback in using the STATIC alternative? Prefixing the function names could make the native API more messy than it needs to be (if someone wants to use the native C spice API for anything other than kernel handling) and I think it's fine if the functions in spice_rub.c aren't visible during compile time. (but I know too little about this to be sure)

translunar commented 8 years ago

Nope, NMatrix API is available in C or C++.

I think static is great. I use it every chance I get. The drawback is that functions you declare static won't be accessible from other object files — but that's also the main feature.

You'll probably still need to prefix functions you can't afford to declare static.

gtamba commented 8 years ago

changed to static

gtamba commented 8 years ago

@mohawkjohn @v0dro

Since static members aren't available to other sources during compilation, I can't modularize the extension code by placing it in separate files because attachment of functions to top level modules occurs only within spice_rub.c .

This is going to make spice_rub.c a rather large file in itself, something that came to light during the code review period. Is there any workaround for this?

translunar commented 8 years ago

Most likely. You can put sources in individual files and then pipe them all together as part of compilation. I'm not sure I've tried this before with Ruby C extensions.

You can also separate things into individual modules, which can each have their own source files, and then combine those in Ruby land. That may be easier and more Ruby-like.

gtamba commented 8 years ago

Removed static and prefixed all C functions. commit #59