search

r-rust / hellorust

Template R package with rust bindings
Other
258 stars 20 forks source link

Remove redundant `extern` storage class specifier #3

Closed klmr closed 6 years ago

klmr commented 6 years ago

extern in C and C++ is used to syntactically distinguish declarations from definitions for the purpose of linkage. This isn’t necessary for function declarations: they always have external linkage (unless declared static or in an unnamed namespace).

Since the rules are confusing, I suggest removing the redundant qualifier, as its presence here falsely suggests that it is necessary and confers semantic meaning.

jeroen commented 6 years ago

Thanks for the suggestion. What if we would call it from C++? Should we use something like this: https://github.com/ImageOptim/gifski/commit/555d3dc845b5b3b09180835958b594f7dd57123d

klmr commented 6 years ago

For a multi-purpose library, yes, the extern "C" declaration would be necessary so that it can be called from C++ (but do note that extern "C" is unrelated to plain extern, confusingly).

But since this is a package-intern object .h file, it will only ever be needed to compile the C wrapper for R, won’t it?

jeroen commented 6 years ago

Right, we only have a C wrapper, but the purpose of this package is to provide a general template that people can use as a starting point if they want to wrap their own crates. So it should preferably generalize to people that wish to call rust from C++, without figuring out technical details.

Anyway I'm boarding a flight now, I'll have a look tonight. Thanks!

klmr commented 6 years ago

Fair point! I’ve added a commit to the PR.