michaelh requested changes to this revision.
michaelh added a comment.
This revision now requires changes to proceed.
It would be great if you could add tests for `==`, `:=` and `((...)...)` to
`autotests/unit/lib/advancedqueryparsertest.cpp`
INLINE COMMENTS
> advancedqueryparser.cpp:35
> {
> QStringList tokens;
> QString token;
Suggestion: Rename to `tokenList`. It's less likely to confuse it with `token`
that way.
> advancedqueryparser.cpp:50
> + // Spaces end tokens
> if (token.size() > 0) {
> tokens.append(token);
Please use `!token.isEmpty()`
(Don't exactly know why, but have myself been told so several times. Maybe it's
faster. :-)
> advancedqueryparser.cpp:54
> }
> -
> - // Operators are tokens themselves
> - if (isOperator(c)) {
> - if (tokens.size() > 1) {
> - QString last = tokens.last();
> - if (last.size() == 1 && isOperator(last[0])) {
> - last.append(c);
> - tokens[tokens.size() - 1] = last;
> - continue;
> - }
> - }
> - tokens.append(QString(c));
> + } else if (c == '(' || c == ')') {
> + // Parentheses end tokens, and are tokens by themselves
Please use QLatin1Char
> advancedqueryparser.cpp:55
> + } else if (c == '(' || c == ')') {
> + // Parentheses end tokens, and are tokens by themselves
> + if (token.size() > 0) {
// Parentheses delimit tokens, and are tokens by themselves
> advancedqueryparser.cpp:61
> + tokens.append(c);
> + } else if (c == '>' || c == '<' || c == ':' || c == '=') {
> + // Operators end tokens
Please use QLatin1Char
> advancedqueryparser.cpp:63
> + // Operators end tokens
> + if (token.size() > 0) {
> + tokens.append(token);
Please use `!token.isEmpty()`
> advancedqueryparser.cpp:68
> + // accept '=' after any of the above
> + if (text.at(i + 1) == '=') {
> + tokens.append(text.mid(i, 2));
Please use QLatin1Char
> advancedqueryparser.cpp:69
> + if (text.at(i + 1) == '=') {
> + tokens.append(text.mid(i, 2));
> + i++;
':' means `Term::Contains` and '=' means `Term::Equal` so ':=' is a little
ambigous.
Unless you have a particular reason to interpret ':=' as '=' we should take it
as ':'.
It may be better to have and extra `If` for '=' and ':' and simply drop any
second "=" so we come out with ':' or '=' as token.
The `parse()` function checks the second char only in case of '<' or '>'. So
':=' will become ':'. The
2-char tokens should to be added to the `switch` in `parse()` which then also
could be simplified. The distinction between '>' and '>=' is the lexer's job,
right?
REPOSITORY
R293 Baloo
REVISION DETAIL
https://phabricator.kde.org/D11888
To: bruns, #baloo, michaelh
Cc: #frameworks, ashaposhnikov, michaelh, astippich, spoorun, ngraham, alexeymin
