Thanks @rafie, this looks good. Any thoughts @mnunberg, @oranagra, @yossigo, @itamarhaber, @madolson, ...?

However @rafie, do I understand correctly that this no longer uses the trick of using function pointers that we fill with our implementations? So it's now the linker that will bind the symbols? Are you sure this is as flexible and future-proof as the original approach? We guarantee ABI compatibility in the future. By filling the function pointers manually, we will be able to do many tricks to guarantee the compatibility in the future, I'm not sure if we'll be able to do the same with this new way.

Btw it should be possible to have the same advantages without removing the function pointer approach, just by declaring them extern in the successive inclusions, but not the first.

Oh probably it still uses the function pointers, I just don't understand the popmacro/pushmacro stuff, I'll have to check better.

Wasn't sure i wanted to comment, but since you asked...
Here are few thoughts that came to me while reading:

• as far as i can tell pragma push_macro is not standard
• what's the deal with misc1.c and the makefile complications around it? is it just to test multiple source files in one binary? not sure we need to test it. (unless you think it is at risk of getting re-broken somehow without us noticing it)
• i see this commit also includes re-styling of the code, something that should be in a commit of it's own

Now to the elephant in the room:
the REDISMODULE_XAPI_STABLE give me nausea (although truth be told i had it since i saw the words "C++"), the syntax of seeing the function name and return value / arguments in a non C syntax, listed as arguments for a macro is making my brain freeze for a moment.
i don't like the fact that today we need to add each api in 4 places, but i'm not sure this is worth it.

instead we can keep the old way, and just add macros around the function declarations so that we can control their attributes before including the header file, i think this is the common way this type thing are handled.

i.e. i would also argue that any attributes like week or others should be the responsibility of whoever includes it, not the header itself (maybe the same can be said about extern "C")

for instance, change redismodule.h to something the lines of:

REDIS_MODULE_PRE_API void *(*RedisModule_Alloc)(size_t bytes) REDISMODULE_POST_API;

i.e. dropping the REDISMODULE_API_FUNC macro too (which also bothers me causing the function declarations to be non C syntax).
The syntax of function pointers in C is complicated enough even without adding macros and extra parenthesis into them.

so then, when including the header, you can do:

#define REDIS_MODULE_PRE_API __attribute__ ((weak))
#include "redismodule.h"

p.s. maybe we can unify the list that uses REDISMODULE_GET_API and REGISTER_API into one list using some macro, one that just lists the API names and doesn't mess with their prototypes.

Ok @oranagra, I completely trust your opinion here. So... let's try with baby steps. What about resorting to the PR from @mnunberg, but perhaps also dumbing it down more to just have a way to include the file as extern without the other changes? So that we incrementally start to get better without doing very hard changes.

@antirez It still use same method of assigning function pointers.
The difference is that we're currently doing (for each C file including redismodule.h):

void *(*RedisModule_Alloc)(size_t bytes);
//...

static int RedisModule_Init(RedisModuleCtx *ctx, const char *name, int ver, int apiver) {
    void *getapifuncptr = ((void**)ctx)[0];
    RedisModule_GetApi = (int (*)(const char *, void *)) (unsigned long)getapifuncptr;
    RedisModule_GetApi("RedisModule_Alloc", (void **)&RedisModule_Alloc);
    //...
}

(which causes the linked to complain about multiple symbol definitions.)

The PR is suggesting (for each C file including redismodule.h):

__attribute__ ((weak)) void * (*RedisModule_Alloc)(size_t bytes) = NULL;

static int RedisModule_Init(RedisModuleCtx *ctx, const char *name, int ver, int apiver) {
    RedisModule_GetApiFunctionType getapifuncptr = (RedisModule_GetApiFunctionType)((void **)ctx)[0];

    getapifuncptr("RedisModule_Alloc", (void *)&RedisModule_Alloc);
    //...
}

The __attribute__((weak)) thing makes the linker drop the duplicate API definitions.

As for the pragma push_macro, this is not essential. We can #define __funny_name_X instead of #define X, and undefine it afterwards. The push/pop-macro just makes it cleaner.

As far as aesthetics go, I find it quite reasonable to use a mata-macro that is given type, function name, and argument list, and applies them in different forms.

We can devide-and-conquer as follows:
(1) Decide whether or not to take the X-macros (this will reduce API mentions from 3 to 1).
(2) Decide how to use __attribute__((weak)). For platforms that do not support weak symbols, we can use the following scheme:

In the .c file that's calling RedisModule_OnLoad(), we define REDISMODULE_API_DEFS as follows:

#define REDISMODULE_API_DEFS
#include "redismodules.h"

For this .c file, API vars are defined, and a locally-defined RedisModule_Init() initializes them.
For other .c files that include redismodules.h without REDISMODULE_API_DEFS, API vars are declared as extern.

For platforms that support weak symbols, we go on with the suggested scheme.

We can also encourage all users to convert to the REDISMODULE_API_DEFS scheme, as it offers a cleaner solution.

@rafie I'm not a fan of having something defined before including a header file.. it results in more cruft/clutter, or it requires a dedicated file whose purpose it is to only include that file.

@oranagra 's solution is probably the cleanest. I'm not a fan of having all the API declarations as X-macros either. It does reduce redundancy, but at the cost of making it even more difficult to read the API. I think it should be good if we have at least one "proper" declaration.

I am a bit partial to my own implementation as I think it provides a happy medium between readability and eliminating redundancy.

Not a fan of using weak.

for what it worth, i support the concept of REDISMODULE_API_DEFS that controls extern declarations.

i'm personally against the X-macros, i think the prototype declarations should include pre-processor replacements, but not macro (functions). i suggest to consider using such macro to just reduce the repetition of REDISMODULE_GET_API and REGISTER_API, but not the prototypes.

I also feel that we should avoid using attributes and pragmas in the header since they may be non-standard, but i do think we must allow these to be added by pre-processor at the control of whoever includes the header file.

so i would go with something like

#if !defined REDIS_MODULE_PRE_API and defined REDISMODULE_API_DEFS
#define REDIS_MODULE_PRE_API extern
#endif

REDIS_MODULE_PRE_API void *(*RedisModule_Alloc)(size_t bytes) REDISMODULE_POST_API;

this way the caller can either add extern (rather easily), or add whatever attribute he wants even if non-standard.

@mnunberg can you post a link to your approach, i don't recall what it was.

@mnunberg In a Redis module, there's a single file that's defines RedisModule_OnLoad(). I think it's no big burden to have that file to define REDISMODULE_API_DEFS. I consider that more elegant than having RedisModule_Init() compiled into every compilation unit.
It is also more C-like to have API vars declared extern than using weak symbols.

@oranagra Wouldn't adding REDIS_MODULE_PRE_API and REDIS_MODULE_PRE_API for more than 200 APIs harm readability?
Note that with x-macros we lost the RedisModule_ prefix, which somewhat improved readability.

From my experience the pre/post preprocessor defs is a common pattern and I ignore them when reading since I know they're adding minor modifier keywords, so they don't affect the readability for me. (maybe also because I have a wide monitor?)

What I didn't like in the current PR (and even the original code) is the extra parenthesis and commas that make it looks unlike a C syntax, and in my eyes the PRE and POST preprocessor are not bothering me as much since i can ignore them as if undefined and the syntax is still C.

Regarding dropping the RediszModule_ prefix, I actually consider that a bad thing, if I grep the code for the API name, I won't find it, and I assume some IDEs may have issues with that too (both the missing prefix and the macro magic can cause that).
I suppose the pre/post thing won't cause any such harm to an IDE since it is a common practise (seen it in many libraries and SDKs in the past).

I guess all the arguments on these two approaches are already here, I thinks it's time to either hear some new options, or wait for Salvatore to decide.

The way I see it, the core problems we're trying to solve here are:

1. C++ Support.
2. Multi-file module support.

I think the simplest fix for these issues is clear, i.e. extern "C" and an explicit #define to control which compilation unit gets the declaration. Everything else is basically just a matter of restructuring for personal taste.

My suggestion is to apply the simplest fix which is less likely to have compatibility issues (weak symbols) or break existing modules.

I do think there are possible major improvements for redismodule.h (at least per my personal taste) but they'd anyway involve breaking binary compatibility and should be handled separately.

In a nutshell, what I'd be happy to see is this:

1. No function pointers declared in a header file - everything is extern.
2. A single module API struct that has all function pointers.
3. On initialization users get a pointer to the struct; It's up to them to decide if it's stored as a global, inside some other context struct, passed around on the stack, etc.
4. Can still offer some level of source level backwards compatibility (but not binary) with macros that map RedisModule_Function() to _global_module_api_context->Function().

But again, I think this is a separate future discussion.

@yossigo The all-containing-struct options was considered - it adds an extra dereference for every API call, and does not affect the question of initializing API vars:
it's the same for 220 APIs or for a single struct pointer.

Here's a refinement:
We'll introduce three controlling macros: NO_WEAK_SYMBOLS, REDISMODULEAPI_NO_WEAKSYM, and REDISMODULE_API_DEFS.
If NO_WEAK_SYMBOLS is not explicitly given, we continue with the suggested scheme.
In this case, user may provide a REDISMODULEAPI_WEAKSYM to override the default value of __attribute__((weak)).

If NO_WEAK_SYMBOLS if defined, then we use extern to declare API vars and require REDISMODULE_API_DEFS to be defined for API vars initialization code to be inserted by redismodules.h.
The REDISMODULEAPI_NO_WEAKSYM allows transformation into the REDISMODULE_API_DEFS for all platforms.

Thus, for most users, no code change is required and redismodules.h can be now included from multiple .c files.
For users without weak symbols support, an addition of #define REDISMODULE_API_DEFS in a single file will be required.
For users that wish to transform to the REDISMODULE_API_DEFS, defining REDISMODULEAPI_NO_WEAKSYMwill replace weak symbols withextern` and API initialization will be done in a single file.

Bottom line:

• We allow inclusion of redismodules.h in multiple .c files with no code changes in part of the user in majority of platforms.
• We encorage users to migrate to the REDISMODULE_API_DEFS scheme, by adding REDISMODULEAPI_NO_WEAKSYM in Makefile and REDISMODULE_API_DEFS in main module .c file.

The decision regarding X-macros is orthogonal to this. Using them, however, will make implementation easier.
@oranagra's observation regarding IDEs still holds.

@rafie I think the additional pointer de-reference is negligible, and it does change the initialization matter because redismodule.h doesn't need to declare any variable. All it needs something like

struct RedisModuleAPI {
  FunctionType MyFunction();
};

extern struct RedisModuleAPI *RedisModule_Init(...);

But as I stated before, I would raise this only as a far future option.

Why go through the trouble of supporting both weak symbols and no weak symbols? What do we tell module developers to use, in particular if we want the module to compile in both cases? Feels like it's just pushing the problem to the module.

Closing this one in favor of #6900. If you can, please test the changes in that PR and acknowledge that they indeed solve the problem. please post feedback in #6900.