Hi folks,
I'm proposing a small policy change for the way we refer to boolean Kconfig variables in code. Right now, the directive is to always use the IS_ENABLED() macro. I would like to replace this with a shorter macro CONFIG() that also removes the need to type the CONFIG_ prefix again. The idea is mostly to save some horizontal space and the occasional extra line break for this boilerplate and make it a little easier to type.
It's a very simple change (patch train starts at https://review.coreboot.org/c/coreboot/+/31773), but since it affects pretty much all code in coreboot and payloads I wanted to send out a quick FYI. Please let me know if anyone has any concerns with this.
On Wed, Mar 6, 2019 at 3:52 AM Julius Werner jwerner@chromium.org wrote:
Hi folks,
I'm proposing a small policy change for the way we refer to boolean Kconfig variables in code. Right now, the directive is to always use the IS_ENABLED() macro. I would like to replace this with a shorter macro CONFIG() that also removes the need to type the CONFIG_ prefix again. The idea is mostly to save some horizontal space and the occasional extra line break for this boilerplate and make it a little easier to type.
I like it.
It's a very simple change (patch train starts at
https://review.coreboot.org/c/coreboot/+/31773), but since it affects pretty much all code in coreboot and payloads I wanted to send out a quick FYI. Please let me know if anyone has any concerns with this.
While doing so, I think we should consider if some of these CONFIG_xx options are ones we should resolve runtime. Taking ACPI S3 feature as sample, we have acpi_s3_resume_is_allowed() to wrap IS_ENABLED(CONFIG_HAVE_ACPI_RESUME). So if one wanted to provide eg. CMOS bit to disable this, there is less source to change.
There is some indirect CONFIG_xxx changes when you switch payload in config. IMO it should be necessary to simply swap the payload binary in CBFS without rebuilding coreboot proper, so these should also resolve runtime, not build-time. (Sure, we would then need to peek early in process what payload we are going to boot.)
Kyösti
I like it too
On Tue, Mar 5, 2019 at 8:21 PM Kyösti Mälkki kyosti.malkki@gmail.com wrote:
On Wed, Mar 6, 2019 at 3:52 AM Julius Werner jwerner@chromium.org wrote:
Hi folks,
I'm proposing a small policy change for the way we refer to boolean Kconfig variables in code. Right now, the directive is to always use the IS_ENABLED() macro. I would like to replace this with a shorter macro CONFIG() that also removes the need to type the CONFIG_ prefix again. The idea is mostly to save some horizontal space and the occasional extra line break for this boilerplate and make it a little easier to type.
I like it.
It's a very simple change (patch train starts at https://review.coreboot.org/c/coreboot/+/31773), but since it affects pretty much all code in coreboot and payloads I wanted to send out a quick FYI. Please let me know if anyone has any concerns with this.
While doing so, I think we should consider if some of these CONFIG_xx options are ones we should resolve runtime. Taking ACPI S3 feature as sample, we have acpi_s3_resume_is_allowed() to wrap IS_ENABLED(CONFIG_HAVE_ACPI_RESUME). So if one wanted to provide eg. CMOS bit to disable this, there is less source to change.
There is some indirect CONFIG_xxx changes when you switch payload in config. IMO it should be necessary to simply swap the payload binary in CBFS without rebuilding coreboot proper, so these should also resolve runtime, not build-time. (Sure, we would then need to peek early in process what payload we are going to boot.)
Kyösti _______________________________________________ coreboot mailing list -- coreboot@coreboot.org To unsubscribe send an email to coreboot-leave@coreboot.org
Will it be more explicit if we call it HAS_CONFIG(XXX) ? Or HAS_KCONFIG(XXX) CONFIG(XXX) seems too generic to me that some drivers may wan to use it for wrapping a reference to config tables, like GPIO(XXX).
On Wed, Mar 6, 2019 at 9:52 AM Julius Werner jwerner@chromium.org wrote:
Hi folks,
I'm proposing a small policy change for the way we refer to boolean Kconfig variables in code. Right now, the directive is to always use the IS_ENABLED() macro. I would like to replace this with a shorter macro CONFIG() that also removes the need to type the CONFIG_ prefix again. The idea is mostly to save some horizontal space and the occasional extra line break for this boilerplate and make it a little easier to type.
It's a very simple change (patch train starts at https://review.coreboot.org/c/coreboot/+/31773), but since it affects pretty much all code in coreboot and payloads I wanted to send out a quick FYI. Please let me know if anyone has any concerns with this. _______________________________________________ coreboot mailing list -- coreboot@coreboot.org To unsubscribe send an email to coreboot-leave@coreboot.org
On Tue, 2019-03-05 at 17:52 -0800, Julius Werner wrote:
Hi folks,
I'm proposing a small policy change for the way we refer to boolean Kconfig variables in code. Right now, the directive is to always use the IS_ENABLED() macro. I would like to replace this with a shorter macro CONFIG() that also removes the need to type the CONFIG_ prefix again. The idea is mostly to save some horizontal space and the occasional extra line break for this boilerplate and make it a little easier to type.
Sounds like a good plan. Please keep Documentation in sync: Documentation/core/Kconfig.md seems to cover the implementation details.
It's a very simple change (patch train starts at https://review.coreboot.org/c/coreboot/+/31773), but since it affects pretty much all code in coreboot and payloads I wanted to send out a quick FYI. Please let me know if anyone has any concerns with this. _______________________________________________ coreboot mailing list -- coreboot@coreboot.org To unsubscribe send an email to coreboot-leave@coreboot.org
Sounds good to me, too. Then we finally will avoid forgetting the CONIFG_ prefix.
Werner
-----Ursprüngliche Nachricht----- Von: Patrick Rudolph patrick.rudolph@9elements.com Gesendet: Mittwoch, 6. März 2019 08:26 An: Julius Werner jwerner@chromium.org; Coreboot coreboot@coreboot.org Betreff: [coreboot] Re: RFC: Replacing IS_ENABLED(CONFIG_XXX) with the shorter CONFIG(XXX)
On Tue, 2019-03-05 at 17:52 -0800, Julius Werner wrote:
Hi folks,
I'm proposing a small policy change for the way we refer to boolean Kconfig variables in code. Right now, the directive is to always use the IS_ENABLED() macro. I would like to replace this with a shorter macro CONFIG() that also removes the need to type the CONFIG_ prefix again. The idea is mostly to save some horizontal space and the occasional extra line break for this boilerplate and make it a little easier to type.
Sounds like a good plan. Please keep Documentation in sync: Documentation/core/Kconfig.md seems to cover the implementation details.
It's a very simple change (patch train starts at https://review.coreboot.org/c/coreboot/+/31773), but since it affects pretty much all code in coreboot and payloads I wanted to send out a quick FYI. Please let me know if anyone has any concerns with this. _______________________________________________ coreboot mailing list -- coreboot@coreboot.org To unsubscribe send an email to coreboot-leave@coreboot.org
-- Patrick Rudolph
9elements Agency GmbH, Kortumstraße 19-21, 44787 Bochum, Germany Email: patrick.rudolph@9elements.com Phone: +49 234 68 94 188
Sitz der Gesellschaft: Bochum Handelsregister: Amtsgericht Bochum, HRB 17519 Geschäftsführung: Sebastian Deutsch, Daniel Hoelzgen _______________________________________________ coreboot mailing list -- coreboot@coreboot.org To unsubscribe send an email to coreboot-leave@coreboot.org
Sounds like a good plan. Please keep Documentation in sync: Documentation/core/Kconfig.md seems to cover the implementation details.
Uhh... where did you find that one? I don't see it in my tree anywhere...
Will it be more explicit if we call it HAS_CONFIG(XXX) ? Or HAS_KCONFIG(XXX) CONFIG(XXX) seems too generic to me that some drivers may wan to use it for wrapping a reference to config tables, like GPIO(XXX).
Hmm... I would really like to keep it as short as possible, and that's another 4 chars. Also, I feel HAS_CONFIG may be a bit confusing (e.g. may sound more like whether the config is used at all in a particular board/SoC/arch, rather than whether it is enabled). I guess we could go with KCONFIG(XXX) if you prefer, but I kinda liked the symmetry between CONFIG_XXX and CONFIG(XXX).
I would say that the rules for the global namespace are first come, first pick. There are currently no macros named CONFIG() in the tree, and after this is introduced, nobody can add another one (since kconfig.h is force-included in every file and would lead to a duplicate definition). Of course normally we want to avoid names that are too generic in the global namespace, but for something as ubiquitous and useful as this I think we can make an exception (because I doubt any other use case could have a better justification for claiming this name).
(dropped Hung-Te off the list somehow...)
On Wed, Mar 6, 2019 at 12:49 PM Julius Werner jwerner@chromium.org wrote:
Sounds like a good plan. Please keep Documentation in sync: Documentation/core/Kconfig.md seems to cover the implementation details.
Uhh... where did you find that one? I don't see it in my tree anywhere...
Will it be more explicit if we call it HAS_CONFIG(XXX) ? Or HAS_KCONFIG(XXX) CONFIG(XXX) seems too generic to me that some drivers may wan to use it for wrapping a reference to config tables, like GPIO(XXX).
Hmm... I would really like to keep it as short as possible, and that's another 4 chars. Also, I feel HAS_CONFIG may be a bit confusing (e.g. may sound more like whether the config is used at all in a particular board/SoC/arch, rather than whether it is enabled). I guess we could go with KCONFIG(XXX) if you prefer, but I kinda liked the symmetry between CONFIG_XXX and CONFIG(XXX).
I would say that the rules for the global namespace are first come, first pick. There are currently no macros named CONFIG() in the tree, and after this is introduced, nobody can add another one (since kconfig.h is force-included in every file and would lead to a duplicate definition). Of course normally we want to avoid names that are too generic in the global namespace, but for something as ubiquitous and useful as this I think we can make an exception (because I doubt any other use case could have a better justification for claiming this name).
On Thu, Mar 7, 2019 at 4:49 AM Julius Werner jwerner@chromium.org wrote:
Will it be more explicit if we call it HAS_CONFIG(XXX) ? Or
HAS_KCONFIG(XXX)
CONFIG(XXX) seems too generic to me that some drivers may wan to use it
for wrapping a reference to config tables, like GPIO(XXX).
Hmm... I would really like to keep it as short as possible, and that's another 4 chars. Also, I feel HAS_CONFIG may be a bit confusing (e.g. may sound more like whether the config is used at all in a particular board/SoC/arch, rather than whether it is enabled). I guess we could go with KCONFIG(XXX) if you prefer, but I kinda liked the symmetry between CONFIG_XXX and CONFIG(XXX).
CONFIG is better then KCONFIG in that case, thanks! :)
I think it is a good idea, however a lot of under-review changes now have the merge conflicts :P
I think it is a good idea, however a lot of under-review changes now have the merge conflicts :P
Yeah, sorry about that, but that's sorta unavoidable with a change like this.
It's very easy to clean up programmatically though, just run this again:
find src/ -type f | xargs sed -i -e 's/IS_ENABLED\s*(CONFIG_/CONFIG(/g'
Julius Werner jwerner@chromium.org wrote:
It's very easy to clean up programmatically though, just run this again:
find src/ -type f | xargs sed -i -e 's/IS_ENABLED\s*(CONFIG_/CONFIG(/g'
Indeed. The biggest challenge for me was to learn how to correctly resolve these merge conflicts on gerrit, but I finally succeeded with the help of this article - https://www.entropywins.wtf/blog/2013/07/01/resolving-a-merge-conflict-on-ge...
-
Hung-Te Lin -
Julius Werner -
Kyösti Mälkki -
Mike Banon -
Patrick Rudolph -
ron minnich -
Zeh, Werner