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

keyCreate: simpler alternative to keyNew #3112

Closed
kodebach opened this issue Oct 25, 2019 · 6 comments
Closed

keyCreate: simpler alternative to keyNew #3112

kodebach opened this issue Oct 25, 2019 · 6 comments

Comments

@kodebach
Copy link
Member

The API for keyNew is very flexible, but its also very verbose. In addition the resulting binary code is quite big. For example creating a simple key with a string value, type and description metadata is done like this:

keyNew ("/key/name", KEY_VALUE, "string value", KEY_META, "type", "string", KEY_META, "description", "Lorem ipsum dolor sit amet", KEY_END);

The keyswitch_t arguments are not required in this simple case. But each of them represents at least 1 instruction. If a big KeySet is created with many keyNew calls (e.g. in code-generated loadConfiguration like in LCDproc), this can result in many instruction that are somewhat unnecessary. This is why I propose keyCreate and keyVCreate:

Key * keyCreate (const char * name, const char * value, ...);
Key * keyVCreate (const char * name, const char * value, va_list va);

The .../va_list is just a list of 2n+1 const char *. The indices 0, 2, ..., 2n-2 are meta key names, the indices 1, 3, ..., 2n-1 are the corresponding meta key values and 2n is always NULL (n is the number of metakeys).

Not only would these functions reduce the number arguments and therefore the binary size, the could would also be more readable IMO. Especially if keys are created without metakeys (e.g. keyCreate ("/my/key", "my value", NULL)). The functions could also be implemented more efficiently than keyNew.

@dominicjaeger
Copy link
Contributor

I'll just add my example for other readers:

For n=3 we get a list of size 2n+1=7

keyVCreate("/my/key",
       "my value",
0      "firstMeta",
1      "firstMetaValue",
2      "secondMeta",
3      "secondMetaValue",
4=2n-2 "thirdMeta",
5=2n-1 "thirdMetaValue",
6=2n   NULL)

Generally I really like this idea! I just didn't get the difference between keyCreate and keyVCreate or rather ... and va_list va yet. Is the final NULL necessary for the implementation in C?

@markus2330 markus2330 added this to the 2.0.0 milestone Oct 26, 2019
@markus2330
Copy link
Contributor

Thank you for the suggestion! I also like the idea but I need to be strict about not implementing new features as we will get lost otherwise and never finish 1.0. Having keyCreate instead of keyNew is unrealistic to implement.

So, I will close so the issue so that we can focus more on 1.0. As it is tagged with 2.0.0 we can easily find it again once we have released 1.0.0.

@kodebach
Copy link
Member Author

Generally I really like this idea! I just didn't get the difference between keyCreate and keyVCreate or rather ... and va_list va yet.

Its the same as for keyNew and keyVNew. Its mostly an implementation thing.

Is the final NULL necessary for the implementation in C?

Yes, the terminating NULL is required since C doesn't provide a way to find out the number of arguments.

So, I will close so the issue so that we can focus more on 1.0. As it is tagged with 2.0.0 we can easily find it again once we have released 1.0.0.

I totally agree that this is not a 1.0 issue, but since it is just an addition to the API and doesn't break anything we don't have to wait for 2.0.0 either.

@markus2330
Copy link
Contributor

I totally agree that this is not a 1.0 issue, but since it is just an addition to the API and doesn't break anything we don't have to wait for 2.0.0 either.

Yes, good point. If we get too many 2.0.0 issues we can separate between post1.0.0 and things which really need to wait for 2.0.0.

@markus2330
Copy link
Contributor

Btw. every binding writer is free to introduce something like this. For python we will get it now with #3133

@markus2330
Copy link
Contributor

As it came up in #3151. The main reasons to not do it now (for 1.0) is:

  • there is tons of documentation referring to it (also outside of the repo)
  • also other stuff needs to be rewritten, e.g., the c plugin
  • it does not solve the problem with vaargs/bindings
  • it does not allow to create useful aliases like KEY_FUNC

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants