D11888: Handle adjacent special characters correctly

[Michael Heidelbach]

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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20180402/1df36620/attachment-0001.html>