KconfigXT: Add a value attribute to Enum field choices
ClosedPublic

Authored by meven on Feb 17 2020, 3:18 PM.

Details

Summary

Allow to write choices such as :

<choices>
    <choice name="enum_name" value="I can't containt (anything)"></choice>
    <choice name="normal_choice"></choice>
</choices>
Test Plan

ctest

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.
There are a very large number of changes, so older changes are hidden. Show Older Changes
meven updated this revision to Diff 75967.Feb 19 2020, 8:49 AM

Ignore empty value attribute for choice

Could you also add documentation to whatever https://api.kde.org/frameworks/kconfig/html/kconfig_compiler.html is generated from?

meven added a comment.Feb 19 2020, 8:59 AM

Could you also add documentation to whatever https://api.kde.org/frameworks/kconfig/html/kconfig_compiler.html is generated from?

Thanks for pointing it out.

I meant to edit https://techbase.kde.org/Development/Tutorials/Using_KConfig_XT as well after merge.

meven updated this revision to Diff 75968.Feb 19 2020, 9:09 AM

Add documentation about value in dox

meven updated this revision to Diff 75969.Feb 19 2020, 9:13 AM

Add a since to dox

bport added inline comments.Feb 20 2020, 9:01 AM
src/kconfig_compiler/KConfigXmlParser.cpp
206

Can we do a test on more than ' ' value, proably solve other case.
And we probably want to limit what is a correct value, any string seems a bit too large, some character can be problematic when we write back to config file.

meven updated this revision to Diff 76040.Feb 20 2020, 9:44 AM
meven marked an inline comment as done.

Warn user about / . : being forbidden in choice's attribute name

meven added inline comments.Feb 20 2020, 9:44 AM
src/kconfig_compiler/KConfigXmlParser.cpp
206

If you don't mind I am putting aside the second part as it is not directly related to this PR, i.e name attribute validation. (FYI we had KConfigXmlParser::validateNameAndKey)

meven edited the summary of this revision. (Show Details)Feb 20 2020, 9:44 AM
crossi added inline comments.Feb 20 2020, 10:30 AM
src/kconfig_compiler/KConfigXmlParser.cpp
206

I guess there are more problematic characters. IMO this is not the right place, you pointed validateNameAndKey() which seems to be a better choice to implement such checks.

BTW, this is not related to this PR.

ervin requested changes to this revision.Feb 24 2020, 10:56 AM
ervin added inline comments.
src/core/kcoreconfigskeleton.h
790

It's a struct you can drop the public: here.

791

There should be a new line before the opening curly brace.

I also wonder if it wouldn't be better with the implementation being moved to the cpp file, otherwise it risks being inlined which might not be the most convenient for longer term management of the change (if for some reason the implementation needs to be changed).

src/kconfig_compiler/KConfigCommonStructs.h
61

New line before opening curly brace please.

src/kconfig_compiler/KConfigXmlParser.cpp
206

else if should be just after the closing curly brace

As for checking the valid characters for an enum name this is "easy" it can only be alphabetical, numerical and underscore characters (technically shouldn't start with a digit). Any other character will be rejected by the compiler, the current filter is thus not discriminating enough at all and I think its logic should be reversed (the blacklist being potentially infinite it should be white list based).

This revision now requires changes to proceed.Feb 24 2020, 10:56 AM
meven updated this revision to Diff 76405.Feb 25 2020, 6:20 PM
meven marked 6 inline comments as done.

Move KCoreConfigSkeleton::ItemEnum::Choice::value to cpp file, use a regex to filter valid choice name

meven added inline comments.Feb 25 2020, 6:21 PM
src/core/kcoreconfigskeleton.h
788

Something I have noticed while testing this.
Since it changes the memory of a very common data struct in a very common lib, it creates a lot of crashes if apps are not compiled with the installed version.
So all local builds should be rebuild or reinstalled, once this has landed, or you end up with a lot of crashes.

ervin added inline comments.Feb 26 2020, 9:47 AM
src/core/kcoreconfigskeleton.h
788

Oh right, stupid me, this obviously breaks binary compatibility, we need to find another way to store this.

ervin requested changes to this revision.Feb 26 2020, 10:05 AM
ervin added inline comments.
src/core/kcoreconfigskeleton.h
788

Most likely place to hide it would be inside KConfigSkeletonItem's d-pointer (likely a hash associating vals with names)... it'd be unused by most item types of course which sucks but at least it'd be safe BC wise.

src/kconfig_compiler/KConfigXmlParser.cpp
203

We should move the QRegularExpression out of the loop, otherwise it'll get compiled over and over for each choice (premature pessimisation imo). We could go one step further and have it as a member of the parser to have it compiled only once of course, but maybe we'd get in premature optimization territory.

This revision now requires changes to proceed.Feb 26 2020, 10:05 AM
meven updated this revision to Diff 76446.Feb 26 2020, 10:43 AM

Move construct of QRegularExpression out of loop

meven marked an inline comment as done.Feb 26 2020, 10:44 AM
meven updated this revision to Diff 76849.Mar 3 2020, 1:49 PM

Move values attribute for ItemEnum::Choice to KCoreConfigSkeleton::ItemEnum::ItemEnum::mValues to preserve Binary compatiblity

meven updated this revision to Diff 76856.Mar 3 2020, 2:38 PM

Fix no value set case

meven updated this revision to Diff 76866.Mar 3 2020, 4:23 PM

Improve naming, fix case of normal entries, adding a test for this case

meven added a comment.Mar 3 2020, 4:24 PM

I fixed an issue when testing this with D27477 and it is fixed and the issue it revealed is tested.

ervin requested changes to this revision.Mar 5 2020, 8:54 AM
ervin added inline comments.
src/core/kcoreconfigskeleton.cpp
595

Will need an update

601

This comment should go away

src/core/kcoreconfigskeleton.h
812

const QString &name

Probably worth adding a KF6 comment somewhere as well, because your first attempt felt more natural, we're going for this weird construct only for BC concerns.

817

const ref for the parameters

821

Nope, can't be here, this still breaks binary compatibility. As I mentioned in my last comment it should be buried all the way into the d-pointer inherited from KConfigSkeletonItem... This is inefficient in term of memory load but it's the only option to keep BC.

src/kconfig_compiler/KConfigCommonStructs.h
113

Well, on that side you should have kept the more natural value() method IMO.

src/kconfig_compiler/KConfigXmlParser.cpp
193

nitpick: I'd const auto that one, but it's your call

This revision now requires changes to proceed.Mar 5 2020, 8:54 AM
meven updated this revision to Diff 76999.Mar 5 2020, 9:41 AM
meven marked 9 inline comments as done.

Remove enumValues, const ref args, move mValues to KCoreConfigSkeletonItemPrivate, improve comments

meven added inline comments.Mar 5 2020, 9:44 AM
src/core/kcoreconfigskeleton.cpp
595

You mean I should prepare this and put it in KF6 waiting for merge queue ?

src/core/kcoreconfigskeleton.h
812

I have one in QString KCoreConfigSkeleton::ItemEnum::valueForChoice(QString name) const

ervin added inline comments.Mar 5 2020, 10:22 AM
src/core/kcoreconfigskeleton.cpp
595

No I meant the comment needs to be adjusted due to the changes (fields changing place and such) sorry if I was unclear.

src/core/kcoreconfigskeleton.h
812

Yeah, noticed only later... I wonder if it's more obvious in the header or the cpp...

meven updated this revision to Diff 77006.Mar 5 2020, 10:29 AM

Fix typo

meven marked 4 inline comments as done.Mar 5 2020, 10:32 AM
meven added inline comments.
src/core/kcoreconfigskeleton.cpp
595

I updated the comment since your first comment, I think it is ok now.

src/core/kcoreconfigskeleton.h
812

I expect those to be found through grep, and I had comments in the past to put it somewhere where it could not get in documentation, in cpp guarantees this.

ervin requested changes to this revision.Thu, Mar 5, 3:12 PM
ervin added inline comments.
src/core/kcoreconfigskeleton.cpp
596

You mean KConfigSkeletonItemPrivate aren't you? (instead of KCoreConfigSkeletonPrivate)

src/core/kcoreconfigskeleton.h
812

Well, regular comments don't go in the docs ;-)
(you need the triple slash or the double start at start of comment for it to be picked up by doxygen)

But cpp, why not.

src/kconfig_compiler/KConfigCommonStructs.h
60

New line before opening curly brace please

This revision now requires changes to proceed.Thu, Mar 5, 3:12 PM
meven updated this revision to Diff 77064.Fri, Mar 6, 8:11 AM
meven marked 2 inline comments as done.

Fix comment and formatting

ervin accepted this revision.Fri, Mar 6, 10:22 AM

LGTM, please fix the typo in the docs before pushing though

src/kconfig_compiler/README.dox
372

typo: it says "used used"

This revision is now accepted and ready to land.Fri, Mar 6, 10:22 AM
meven updated this revision to Diff 77077.Fri, Mar 6, 10:25 AM

Fix typo

ervin accepted this revision.Fri, Mar 6, 10:25 AM
meven updated this revision to Diff 77225.Sun, Mar 8, 6:01 PM

Rebase on master

This revision was automatically updated to reflect the committed changes.