ervin requested changes to this revision.
ervin added inline comments.
This revision now requires changes to proceed.
INLINE COMMENTS
> kcoreconfigskeleton.h:766
> +
> + public:
> + QString value() const {
It's a struct you can drop the public: here.
> kcoreconfigskeleton.h:767
> + public:
> + QString value() const {
> + return val.isEmpty() ? name : val;
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).
> KConfigCommonStructs.h:61
> + public:
> + QString value() const{
> + return val.isEmpty() ? name : val;
New line before opening curly brace please.
> KConfigXmlParser.cpp:203
> }
> + else if (choice.name.contains(QLatin1Char(' ')) ||
> choice.name.contains(QLatin1Char('/')) ||
> choice.name.contains(QLatin1Char('.')) ||
> choice.name.contains(QLatin1Char(':'))) {
> + std::cerr << "Tag <choice> attribute 'name' must be compatible
> with Enum naming. name was '" << qPrintable(choice.name) << "'. You can use
> attribute 'value' to pass any string as the choice value." << std::endl;
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).
REPOSITORY
R237 KConfig
REVISION DETAIL
https://phabricator.kde.org/D27463
To: meven, ervin, bport, crossi, #frameworks
Cc: ngraham, davidre, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh,
bruns
