Allow to write choices such as :
<choices> <choice name="enum_name" value="I can't containt (anything)"></choice> <choice name="normal_choice"></choice> </choices>
ervin | |
bport | |
crossi |
Frameworks |
Allow to write choices such as :
<choices> <choice name="enum_name" value="I can't containt (anything)"></choice> <choice name="normal_choice"></choice> </choices>
ctest
No Linters Available |
No Unit Test Coverage |
Buildable 22928 | |
Build 22946: arc lint + arc unit |
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.
src/kconfig_compiler/KConfigXmlParser.cpp | ||
---|---|---|
205 | Can we do a test on more than ' ' value, proably solve other case. |
src/kconfig_compiler/KConfigXmlParser.cpp | ||
---|---|---|
205 | 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) |
src/kconfig_compiler/KConfigXmlParser.cpp | ||
---|---|---|
205 | 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. |
src/core/kcoreconfigskeleton.h | ||
---|---|---|
766 ↗ | (On Diff #76040) | It's a struct you can drop the public: here. |
767 ↗ | (On Diff #76040) | 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 ↗ | (On Diff #76040) | New line before opening curly brace please. |
src/kconfig_compiler/KConfigXmlParser.cpp | ||
203 ↗ | (On Diff #76040) | 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). |
Move KCoreConfigSkeleton::ItemEnum::Choice::value to cpp file, use a regex to filter valid choice name
src/core/kcoreconfigskeleton.h | ||
---|---|---|
764 ↗ | (On Diff #76040) | Something I have noticed while testing this. |
src/core/kcoreconfigskeleton.h | ||
---|---|---|
764 ↗ | (On Diff #76040) | Oh right, stupid me, this obviously breaks binary compatibility, we need to find another way to store this. |
src/core/kcoreconfigskeleton.h | ||
---|---|---|
764 ↗ | (On Diff #76040) | 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 | ||
202 | 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. |
Move values attribute for ItemEnum::Choice to KCoreConfigSkeleton::ItemEnum::ItemEnum::mValues to preserve Binary compatiblity
I fixed an issue when testing this with D27477 and it is fixed and the issue it revealed is tested.
src/core/kcoreconfigskeleton.cpp | ||
---|---|---|
580 ↗ | (On Diff #76040) | Will need an update |
586 ↗ | (On Diff #76040) | This comment should go away |
src/core/kcoreconfigskeleton.h | ||
792 ↗ | (On Diff #76040) | 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. |
793 ↗ | (On Diff #76040) | 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. |
798 ↗ | (On Diff #76040) | const ref for the parameters |
src/kconfig_compiler/KConfigCommonStructs.h | ||
113 ↗ | (On Diff #76040) | Well, on that side you should have kept the more natural value() method IMO. |
src/kconfig_compiler/KConfigXmlParser.cpp | ||
194 ↗ | (On Diff #76040) | nitpick: I'd const auto that one, but it's your call |
Remove enumValues, const ref args, move mValues to KCoreConfigSkeletonItemPrivate, improve comments
src/core/kcoreconfigskeleton.cpp | ||
---|---|---|
580 ↗ | (On Diff #76040) | 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 | ||
793 ↗ | (On Diff #76040) | Yeah, noticed only later... I wonder if it's more obvious in the header or the cpp... |
src/core/kcoreconfigskeleton.cpp | ||
---|---|---|
580 ↗ | (On Diff #76040) | I updated the comment since your first comment, I think it is ok now. |
src/core/kcoreconfigskeleton.h | ||
793 ↗ | (On Diff #76040) | 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. |
src/core/kcoreconfigskeleton.cpp | ||
---|---|---|
581 ↗ | (On Diff #76040) | You mean KConfigSkeletonItemPrivate aren't you? (instead of KCoreConfigSkeletonPrivate) |
src/core/kcoreconfigskeleton.h | ||
793 ↗ | (On Diff #76040) | Well, regular comments don't go in the docs ;-) But cpp, why not. |
src/kconfig_compiler/KConfigCommonStructs.h | ||
60 ↗ | (On Diff #76040) | New line before opening curly brace please |
LGTM, please fix the typo in the docs before pushing though
src/kconfig_compiler/README.dox | ||
---|---|---|
372 ↗ | (On Diff #76040) | typo: it says "used used" |