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

IPC commands with multiple arguments #20

Open
bakkeby opened this issue Mar 8, 2021 · 2 comments
Open

IPC commands with multiple arguments #20

bakkeby opened this issue Mar 8, 2021 · 2 comments

Comments

@bakkeby
Copy link
Contributor

bakkeby commented Mar 8, 2021

I thought I'd make some notes about IPC commands with multiple arguments as it is not particularly intuitive and is likely going to confuse some people. Or perhaps I am just doing something wrong.

The various compilation errors may also depend on the compiler used, not sure.

In any case after some trial end errors I ended up with a function like this:

void
mwahaha(const Arg args[], int num_args)
{
	fprintf(stderr, "sure thing, mwahaha ran, num_args = %d\n", num_args);
	fprintf(stderr, "args[0] = %ld\n", args[0].i);
	fprintf(stderr, "args[1] = %ld\n", args[1].i);
}

The first thing I tried which I also feel is most intuitive is to add it like this in the ipcommands array:

IPCCOMMAND( mwahaha, 2, {ARG_TYPE_SINT, ARG_TYPE_SINT} ),

However that gives me a compilation error in relation to passing more arguments than the macro expects (using the gcc compiler).

config.h:768:56: error: macro "IPCCOMMAND" passed 4 arguments, but takes just 3
  768 |  IPCCOMMAND( mwahaha, 2, {ARG_TYPE_SINT, ARG_TYPE_SINT} ),
      |                                                        ^
In file included from lib/include.h:38,
                 from dwm.c:525:
lib/ipc/ipc.h:15: note: macro "IPCCOMMAND" defined here
   15 | #define IPCCOMMAND(FUNC, ARGC, TYPES)                                          \
      |
In file included from dwm.c:575:
config.h:768:2: error: ‘IPCCOMMAND’ undeclared here (not in a function)
  768 |  IPCCOMMAND( mwahaha, 2, {ARG_TYPE_SINT, ARG_TYPE_SINT} ),
      |  ^~~~~~~~~~

While I didn't expect it to work I tried defining the ArgType separately and use that to work around the macro issue

ArgType testarg[2] = {ARG_TYPE_SINT, ARG_TYPE_SINT};

and adding it like this:

IPCCOMMAND( mwahaha, 2, testarg ),

but this led to an error regarding that the cast specifies the array type.

config.h:767:14: warning: initialization of ‘void (*)(const Arg *)’ from incompatible pointer type ‘void (*)(const Arg *, int)’ [-Wincompatible-pointer-types]
  767 |  IPCCOMMAND( mwahaha, 2, testarg ),
      |              ^~~~~~
lib/ipc/ipc.h:16:13: note: in definition of macro ‘IPCCOMMAND’
   16 |   { #FUNC, {FUNC }, ARGC, (ArgType[ARGC])TYPES }
      |             ^~~~
config.h:767:14: note: (near initialization for ‘ipccommands[0].func.single_param’)
  767 |  IPCCOMMAND( mwahaha, 2, testarg ),
      |              ^~~~~~
lib/ipc/ipc.h:16:13: note: in definition of macro ‘IPCCOMMAND’
   16 |   { #FUNC, {FUNC }, ARGC, (ArgType[ARGC])TYPES }
      |             ^~~~
lib/ipc/ipc.h:16:27: error: cast specifies array type
   16 |   { #FUNC, {FUNC }, ARGC, (ArgType[ARGC])TYPES }
      |                           ^
config.h:767:2: note: in expansion of macro ‘IPCCOMMAND’
  767 |  IPCCOMMAND( mwahaha, 2, testarg ),
      |  ^~~~~~~~~~

The last thing I tried was to skip the macro entirely and add the IPC command manually:

static IPCCommand ipccommands[] = {
	{ "mwahaha", { mwahaha }, 2, (ArgType[2]){ARG_TYPE_SINT, ARG_TYPE_SINT} },
}

This still adds a warning as the function signature does not match the signature of the single param function, but it still compiles fine.

config.h:766:16: warning: initialization of ‘void (*)(const Arg *)’ from incompatible pointer type ‘void (*)(const Arg *, int)’ [-Wincompatible-pointer-types]
  766 |  { "mwahaha", { mwahaha }, 2, (ArgType[2]){ARG_TYPE_SINT, ARG_TYPE_SINT} },
      |                ^~~~~~
config.h:766:16: note: (near initialization for ‘ipccommands[0].func.single_param’)

and executes fine:

$ dwm-msg run_command mwahaha 3 5
{"result":"success"}

Output:

Received EPOLLIN event on socket
sure thing, mwahaha ran, num_args = 2
args[0] = 3
args[1] = 5

and finally I was able to silence the warning during compilation by adding these pragmas:

#pragma GCC diagnostic push
#pragma GCC diagnostic ignored "-Wincompatible-pointer-types"
static IPCCommand ipccommands[] = {
	{ "mwahaha", { mwahaha }, 2, (ArgType[2]){ARG_TYPE_SINT, ARG_TYPE_SINT} },
}
#pragma GCC diagnostic pop

I hope this is useful to someone and let me know if there is a better way of doing this.

@mihirlad55
Copy link
Owner

Thanks for the detailed analysis. I hadn't done much testing with multiple arguments as I did not have a need to use them. But this is definitely unintuitive in its current state and does not really work with the C Macro because of the way the pre-processor works. There seem two be two main problems with adding multiple commands:

  1. The C preprocessor does not like commas within a single argument. It sees a comma as a delimiter for the macro itself.
  2. The compiler tries to automatically use the single_param member for the ArgType union instead of array_param member even for multi-argument commands.

I think I've come up with a somewhat elegant/hacky solution that solves both of those problems without the need for disabling any compiler warnings that could identify potentially bad/buggy code.

The following macro should work for multi-argument IPC Command definitions:

#define IPCCOMMAND_MULT(FUNC, ARGC, ...)                                       \
  { #FUNC, {.array_param=FUNC }, ARGC, (ArgType[ARGC])__VA_ARGS__ }

Now the only thing I don't like about this solution, is I believe the pre-processor is treating the first argument as {ARG_TYPE_SINT and the last argument as ARG_TYPE_UINT}. Which is not a huge problem since everything compiles nicely, but it bothers me to know that's what is really happening.

Now to fix this, we could do the following

#define IPCCOMMAND_MULT(FUNC, ARGC, ...)                                       \
  { #FUNC, {.array_param=FUNC }, ARGC, (ArgType[ARGC]){__VA_ARGS__} }

and then drop the braces around types for multi-argument commands. However, this makes the ipccommands array more confusing/unintuitive since multi-argument commands wouldn't have the braces, but single-argument commands would.

We could alter the IPCCOMMAND macro in a similar way

#define IPCCOMMAND(FUNC, ARGC, TYPES)                                          \
  { #FUNC, {FUNC }, ARGC, (ArgType[ARGC]){TYPES} }

and add braces around TYPES. Then the pre-processor is treating each type as a separate argument and then just wrapping all
the types in braces which makes more sense for how macros should be used. Once again, the braces in config.def.h would need to be dropped for the types i.e.

IPCCOMMAND(  quit,                1,      ARG_TYPE_NONE   ),

Now the main issue with altering the IPCCOMMAND macro is that it would break configs for everyone since everyone would need to remove the braces around the types. Though the macros would work better and more elegantly.

So it looks like it will be a trade-off between a bit uglier code and making a config-breaking patch update.

What do you think?

@bakkeby
Copy link
Contributor Author

bakkeby commented Mar 11, 2021

As for the last option that works with single arguments for me, but it doesn't work with multiple arguments.

All in all I think that we shouldn't worry too much about breaking existing configs.

Now, if I keep the pragma of ignoring incompatible pointer types then option 2 works fine for both cases (single and multi param, and named IPCCOMMAND rather than IPCCOMMAND_MULT).

If we do end up with two different macros depending on whether it is a single argument or multiple, then it makes sense taking this a step further and drop the count for the single param.

Example:

#define IPCCOMMAND(FUNC, TYPE)                                                \
  { #FUNC, {.single_param=FUNC }, 1, (ArgType[1]){TYPE} }

#define IPCCOMMANDS(FUNC, ARGC, ...)                                          \
  { #FUNC, {.array_param=FUNC }, ARGC, (ArgType[ARGC]){__VA_ARGS__} }

Then having config like this:

static IPCCommand ipccommands[] = {
	IPCCOMMANDS( customfunc, 8, ARG_TYPE_SINT, ARG_TYPE_STR, ARG_TYPE_SINT, ARG_TYPE_SINT, ARG_TYPE_SINT, ARG_TYPE_SINT, ARG_TYPE_SINT, ARG_TYPE_SINT ),
	IPCCOMMAND( incnmaster, ARG_TYPE_SINT ),
};

As you said you never had any practical use for commands handling multiple arguments and that will be the case for most people. Personally I just have some very specific use cases for this.

I think even if you have to add an IPC command with multiple arguments manually instead of using a macro it would be fine as it is such a rare case. It would just good to have an example of how to do so should one need it.

nuxshed added a commit to nuxshed/dwm that referenced this issue Aug 25, 2021
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

No branches or pull requests

2 participants