Clarification of the code style to use for module* params in Kconfig templates

[pfalcon]

Recently, a "Kconfig template feature" was introduced, which requires assigning values to some template variables and then source a "Kconfig template" which will have those vars substituted.

This ticket is to clarify punctuation style to use of these template var assignments. E.g., initially syntax with spaces around =, e.g. 6aed72e#diff-c329d91124f630eff1649946f71669b8R44 :

module = SHELL
module-str = Shell

However, PR #8816
landed a bunch (17 hits in my grep) of using it without spaces (e.g. https://github.com/zephyrproject-rtos/zephyr/pull/8816/files#r221893651):

module-dep=LOG
module-str=Log level for Ethernet driver
module-help=Sets log level for Ethernet Device Drivers.

So, this ticket is to see if we care and calling to @ulfalizer as the author of templating feature to explain how it's intended to be.

P.S. This may seem minor, but nowaydays we do as insane things as stuffing "(void)" in front of each memset, so surely we can do a sane thing of notwritingstuffwithoutspaces? Also, all big mess starts from small cases like this.

[pfalcon]

cc @jukkar

[ulfalizer]

Wee... a soapbox. ;)

I like spaces around =. Looks less cramped, and avoids tricking people into thinking that whitespace is significant. Preprocessor variables are based on Make variables, and people usually put spaces around = and := there too.

Another thing is that I'd use a plain Kconfig if instead of module-dep:

if LOG

module = SLIP
module-str = Log level for slip driver
source "subsys/net/Kconfig.template.log_config.net"

endif # LOG

It's a few more lines (the blank lines are negotiable), but makes it obvious at a glance that the LOG dependency applies to everything in the template.

In general, I think it's best to avoid using the preprocessor for things that can be handled as easily in plain Kconfig. It just adds another layer for people to untangle.

Being able to define templates like this is more a side effect of the new Kconfig preprocessor by the way. It could easily be abused to create harder-to-read Kconfig files, so we ought to be careful with it.

Oh... and if you need someone sympathetic to whine to about MISRA-related cargo-culting, I'm your guy.~

[pfalcon]

@ulfalizer : Thanks for the detailed reply. Glad that you confirm preferences for spaces around "=", and since posting this ticket, the case which prompted it was resolved: #10406 , i.e. it wasn't the case that there're disagreement, it was just an omission in an otherwise pretty big and backlogged patch.

Another thing is that I'd use a plain Kconfig if instead of module-dep:

if LOG

Hmm, with me actually paying attention to this for the first time, I'd say I personally prefer module-dep - it's pretty useful feature to be able to abstract "control flow" to a template, this kinda makes it more "declarative" than "imperative", and easier to parse/reason about.

Being able to define templates like this is more a side effect of the new Kconfig preprocessor

Thanks, that was a helpful reference to read (e.g. paying attention that in preprocessor function calls, whitespace is significant and apparently the right style is to avoid it ($(func,arg1,arg2)).

Oh... and if you need someone sympathetic to whine to about MISRA-related cargo-culting, I'm your guy.~

Yeah, maybe we can form a dissident faction ;-).

Thanks for a quick consult! Given that, as mentioned above, nobody seems to disagree, let me close this, but it's certainly a useful reference to have around.

[ulfalizer]

Hmm, with me actually paying attention to this for the first time, I'd say I personally prefer module-dep - it's pretty useful feature to be able to abstract "control flow" to a template, this kinda makes it more "declarative" than "imperative", and easier to parse/reason about.

The problem I have it here is that it hides that DEP a dependency for the entire template. If it was just for just a few things in it, it'd seem more okay. Maybe there'd be a simpler way to do it then though.

I like maximally boring code.~

[pfalcon]

The problem I have it here is that it hides that DEP a dependency for the entire template.

That's exactly my point - it hides the implementation details. A chunk like:

module = WIFI
module-dep = LOG
module-str = Log level for WiFi driver
module-help = Sets log level for WiFi Device Drivers.
source "subsys/net/Kconfig.template.log_config.net"

Contains all what's required to configure logging for a particular module, and that's what a user should know. They can of course go to Kconfig.template.log_config.net and check the implementation.

I myself yet need to check how all that looks inside kconfig UI, and most importantly - in raw prj.conf files, because that's what developers deal with mostly, but at least on Kconfig files side, IMHO it's pretty good that way.

[ulfalizer]

That's exactly my point - it hides the implementation details

To me it feels like the Kconfig equivalent of doing

dostuffs(x > 3);

instead of

if (x > 3)
    dostuffs();

That's why I'm a bit skeptical.

I myself yet need to check how all that looks inside kconfig UI, and most importantly - in raw prj.conf files, because that's what developers deal with mostly, but at least on Kconfig files side, IMHO it's pretty good that way.

It's mostly just a dumb textual preprocessor, limited to producing symbol names/values during macro expansion. menuconfig will show the expanded version of everything.

menuconfig and the auto-generated docs show how symbols got included now, so that might make it a bit less confusing. Search for "included via" in https://docs.zephyrproject.org/latest/reference/kconfig/CONFIG_COAP_LOG_LEVEL.html for example.