Changeset View
Standalone View
src/kconfig_compiler/KConfigXmlParser.cpp
| Show First 20 Lines • Show All 170 Lines • ▼ Show 20 Line(s) | 170 | } else { | |||
|---|---|---|---|---|---|
| 171 | std::cerr << "Parameter '" << qPrintable(readEntry.param) << "' has type " << qPrintable(readEntry.paramType) | 171 | std::cerr << "Parameter '" << qPrintable(readEntry.param) << "' has type " << qPrintable(readEntry.paramType) | ||
| 172 | << " but must be of type int, uint or Enum." << std::endl; | 172 | << " but must be of type int, uint or Enum." << std::endl; | ||
| 173 | exit(1); | 173 | exit(1); | ||
| 174 | } | 174 | } | ||
| 175 | } | 175 | } | ||
| 176 | 176 | | |||
| 177 | bool KConfigXmlParser::hasDefaultCode(CfgEntry &readEntry, const QDomElement &element) | 177 | bool KConfigXmlParser::hasDefaultCode(CfgEntry &readEntry, const QDomElement &element) | ||
| 178 | { | 178 | { | ||
| 179 | Q_UNUSED(readEntry) | ||||
| 179 | for (QDomElement e = element.firstChildElement(); !e.isNull(); e = e.nextSiblingElement()) { | 180 | for (QDomElement e = element.firstChildElement(); !e.isNull(); e = e.nextSiblingElement()) { | ||
| 180 | if (e.attribute(QStringLiteral("param")).isEmpty()) { | 181 | if (e.attribute(QStringLiteral("param")).isEmpty()) { | ||
| 181 | if (e.attribute(QStringLiteral("code")) == QLatin1String("true")) { | 182 | if (e.attribute(QStringLiteral("code")) == QLatin1String("true")) { | ||
| 182 | return true; | 183 | return true; | ||
| 183 | } | 184 | } | ||
| 184 | } | 185 | } | ||
| 185 | } | 186 | } | ||
| 186 | return false; | 187 | return false; | ||
| 187 | } | 188 | } | ||
| 188 | 189 | | |||
| 189 | | ||||
| 190 | void KConfigXmlParser::readChoicesFromEntry(CfgEntry &readEntry, const QDomElement &e) | 190 | void KConfigXmlParser::readChoicesFromEntry(CfgEntry &readEntry, const QDomElement &e) | ||
| 191 | { | 191 | { | ||
| 192 | QList<CfgEntry::Choice> chlist; | 192 | QList<CfgEntry::Choice> chlist; | ||
| 193 | const auto choiceNameRegex = QRegularExpression(QStringLiteral("\\w+")); | ||||
ervin: nitpick: I'd const auto that one, but it's your call | |||||
| 194 | | ||||
| 193 | for (QDomElement e2 = e.firstChildElement(); !e2.isNull(); e2 = e2.nextSiblingElement()) { | 195 | for (QDomElement e2 = e.firstChildElement(); !e2.isNull(); e2 = e2.nextSiblingElement()) { | ||
| 194 | if (e2.tagName() != QLatin1String("choice")) { | 196 | if (e2.tagName() != QLatin1String("choice")) { | ||
| 195 | continue; | 197 | continue; | ||
| 196 | } | 198 | } | ||
| 197 | CfgEntry::Choice choice; | 199 | CfgEntry::Choice choice; | ||
| 198 | choice.name = e2.attribute(QStringLiteral("name")); | 200 | choice.name = e2.attribute(QStringLiteral("name")); | ||
| 199 | if (choice.name.isEmpty()) { | 201 | if (choice.name.isEmpty()) { | ||
| 200 | std::cerr << "Tag <choice> requires attribute 'name'." << std::endl; | 202 | std::cerr << "Tag <choice> requires attribute 'name'." << std::endl; | ||
| 203 | } else if (!choiceNameRegex.match(choice.name).hasMatch()) { | ||||
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. ervin: We should move the QRegularExpression out of the loop, otherwise it'll get compiled over and… | |||||
| 204 | 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; | ||||
| 201 | } | 205 | } | ||
| 206 | choice.val = e2.attribute(QStringLiteral("value")); | ||||
Can we do a test on more than ' ' value, proably solve other case. bport: Can we do a test on more than ' ' value, proably solve other case.
And we probably want to… | |||||
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: If you don't mind I am putting aside the second part as it is not directly related to this PR… | |||||
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. crossi: I guess there are more problematic characters. IMO this is not the right place, you pointed… | |||||
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). ervin: else if should be just after the closing curly brace
As for checking the valid characters for… | |||||
| 202 | for (QDomElement e3 = e2.firstChildElement(); !e3.isNull(); e3 = e3.nextSiblingElement()) { | 207 | for (QDomElement e3 = e2.firstChildElement(); !e3.isNull(); e3 = e3.nextSiblingElement()) { | ||
| 203 | if (e3.tagName() == QLatin1String("label")) { | 208 | if (e3.tagName() == QLatin1String("label")) { | ||
| 204 | choice.label = e3.text(); | 209 | choice.label = e3.text(); | ||
| 205 | choice.context = e3.attribute(QStringLiteral("context")); | 210 | choice.context = e3.attribute(QStringLiteral("context")); | ||
| 206 | } | 211 | } | ||
| 207 | if (e3.tagName() == QLatin1String("tooltip")) { | 212 | if (e3.tagName() == QLatin1String("tooltip")) { | ||
| 208 | choice.toolTip = e3.text(); | 213 | choice.toolTip = e3.text(); | ||
| 209 | choice.context = e3.attribute(QStringLiteral("context")); | 214 | choice.context = e3.attribute(QStringLiteral("context")); | ||
| ▲ Show 20 Lines • Show All 348 Lines • Show Last 20 Lines | |||||
nitpick: I'd const auto that one, but it's your call