Following D27059, add parentGroupName attribute to group element to generate kconfig settings with subgroups
Details
Details
- Reviewers
ervin dfaure meven - Group Reviewers
Frameworks - Commits
- R237:00213a3a0906: kconfig_compiler : generate kconfig settings with subgroup
Diff Detail
Diff Detail
- Repository
- R237 KConfig
- Lint
Automatic diff as part of commit; lint not applicable. - Unit
Automatic diff as part of commit; unit tests not applicable.
| src/kconfig_compiler/KConfigSourceGenerator.cpp | ||
|---|---|---|
| 337 | This character won't be easy on the user. | |
| src/kconfig_compiler/KConfigSourceGenerator.cpp | ||
|---|---|---|
| 337 | I'm open for another separator, btw this part is not intended for the user, they have super user friendly KCM to edit their settings. Adding nested <group> as you mentioned has bigger impact on compiler, xsd, and so on. | |
| src/kconfig_compiler/KConfigSourceGenerator.cpp | ||
|---|---|---|
| 337 | Here, I meant kcfg user, i.e dev users, since kconfig is a lib | |
Comment Actions
@dfaure, any opinion on the separator character? It came from me checking which character is forbidden in group names, and it seems that only "group separator" is, so it started a bit like a joke with "I wonder if we'd need to patch KConfigSkeleton at all for subgroups if we pass that in a name", turns out we don't really and seeing how it look in the XML is... meh.
I see two ways out: modifying the schema to be able to specify subgroups in the XML or hijacking a different character that no one use in the wild (which would mean looking at every single kcfg out there... not easy).
Comment Actions
I really have zero experience with this stuff, but you're asking for my opinion nonetheless, so you'll get it, as crappy as it might be :-)
Your comment refers to "group separator" as if it was an existing notion in KConfig. Can you point me where? Grepping for '241' or for 'separator' gives nothing.
Do KConfig files use this already? Or is this all kconfig_compiler specific?
I told you, I'm clueless :-)
Comment Actions
Right, on the C++ side you'd have to look for \x1d which is the "group separator" character in the ascii table, for more information: https://www.asciihex.com/character/control/29/0x1D/gs-group-separator
Comment Actions
OK.
I wonder how/why it becomes ␝ in XML, but quick googling says that entity seems to be '␝' which indeed reads GS :)
At the KConfig level, this is good for catching people who don't use the proper APIs that encapsulate that stuff :-)
But at the kconfig_compiler level, people want to be able to write XML files by hand.
So "modifying the schema to be able to specify subgroups in the XML" sounds like the way to go, not coming up with yet another weird character (it might as well be this one then, for consistency).
Comment Actions
Thanks for your comments.
A new attribute to the group element would be more readable and easier to edit/maintain.
To generate code like :
KConfig config; KConfigGroup generalGroup( &config, "General" ); KConfigGroup colorsGroup = generalGroup.group( "Colors" );
kcfg will look like :
<group name="Colors" parentGroup="General">
<entry name="Foo" type="Bool">
<default>true</default>
</entry>
</group>Currently, I only need one level of subgroup, let's keep it simple and drop the group separator stuff for the moment.
| autotests/kconfig_compiler/kconfigcompiler_test.cpp | ||
|---|---|---|
| 73 | This seems unrelated... So we had this test case available but it was in fact never run? | |
| autotests/kconfig_compiler/test_subgroups.kcfg | ||
| 11 | Now that I see it, I think I'd go for "parentGroupName" since this is not referential and really about the name (like the name parameter) | |
| 21 | Probably worth also having cases with:
Just to have all the possible combinations. | |
| src/kconfig_compiler/KConfigSourceGenerator.cpp | ||
| 349 | Unrelated right, this is not due to your patch, or am I confused? | |
| src/kconfig_compiler/KConfigXmlParser.cpp | ||
| 497 | I'd const auto it, or at least const. | |
Comment Actions
The patch and the feature sound good to me, the resulting xml feels a bit weird and could use some clarification.
| autotests/kconfig_compiler/test_subgroups.kcfg | ||
|---|---|---|
| 22 | It feels odd to reference the parent name rather than having a group inside the other. Why don't we do something like: <group name="Potato">
//entries
<group name="SmallPotato">
//entries
</group>
</group>Might be missing something. | |
Comment Actions
Thank you for your comment.
I agree it looks a bit odd, nested group element would be more xmlish.
We choose a simpler approach as moving in the kconfig_compiler code isn't easy and the need was for one kcm (notifications).