ervin requested changes to this revision.
ervin added inline comments.
This revision now requires changes to proceed.

INLINE COMMENTS

> ervin wrote in kcoreconfigskeleton.h:764
> Oh right, stupid me, this obviously breaks binary compatibility, we need to 
> find another way to store this.

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.

> KConfigXmlParser.cpp:202
>              std::cerr << "Tag <choice> requires attribute 'name'." << 
> std::endl;
> +        } else if 
> (!(QRegularExpression(QStringLiteral("\\w+"))).match(choice.name).hasMatch()) 
> {
> +            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;

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.

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

Reply via email to