Bug 124731 – check-webkit-style is wrong about expected format parameter pack rvalue reference arguments

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Brady Eidson]

check-webkit-style is wrong about expected format parameter pack rvalue reference arguments

Noticed in a patch attached to https://bugs.webkit.org/show_bug.cgi?id=124698

The complaints were:
Source/WebKit2/Shared/AsyncRequest.h:44:  Missing spaces around &&  [whitespace/operators] [3]
Source/WebKit2/Shared/AsyncRequest.h:70:  Missing spaces around &&  [whitespace/operators] [3]
Source/WebKit2/Shared/AsyncRequest.h:91:  Missing spaces around &&  [whitespace/operators] [3]

The lines in question are:
    template<typename... Arguments> void requestCompleted(Arguments&&... arguments);
    void requestCompleted(Arguments&&... arguments)
    template<typename... Arguments> void AsyncRequest::requestCompleted(Arguments&&... arguments)

The script must be detecting && as the 'boolean and' operator.  The formatting in this patch is actually correct/preferred.

[Zan Dobersek]

Bug #123406 likely covers the same issue.

[Brady Eidson]

(In reply to comment #1)
> Bug #123406 likely covers the same issue.

Hmmmm  I guess it's not out on the bot yet?

[Csaba Osztrogonác]

László fixed an rvalue related bug in bug123406. It seems 
the style checker needs a little bit more fine tuning.

[Csaba Osztrogonác]

(In reply to comment #1)
> Bug #123406 likely covers the same issue

Unortunately no, I tried the mentioned patch after the fix
for bug123406 landed, but style checker still fails on it.

[László Langó]

(In reply to comment #4)
> (In reply to comment #1)
> > Bug #123406 likely covers the same issue
> 
> Unortunately no, I tried the mentioned patch after the fix
> for bug123406 landed, but style checker still fails on it.

Yes, that was another bug, but i can check this too, if nobody works on it.

[László Langó]

Created attachment 217661 [details]
Patch

[László Langó]

Created attachment 217663 [details]
Patch

[László Langó]

I fixed this. It passed on the code you mentioned and passed on all webkitpy test, but maybe this isn't the best regexp. I don't know that is there any other tricky way to use c++11 rvaule reference.

[Peter Gal]

Comment on attachment 217663 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=217663&action=review

> Tools/Scripts/webkitpy/style/checkers/cpp.py:1848
>      if matched:
> -        error(line_number, 'whitespace/operators', 3,
> -              'Missing spaces around %s' % matched.group(1))
> +        # It is necessary to check this because rvaule references
> +        # parameter packs (c++11 feature)
> +        if not search(r'&&\.\.\.', line):

These two ifs could be merged together.

[László Langó]

(In reply to comment #9)
> (From update of attachment 217663 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=217663&action=review
> 
> > Tools/Scripts/webkitpy/style/checkers/cpp.py:1848
> >      if matched:
> > -        error(line_number, 'whitespace/operators', 3,
> > -              'Missing spaces around %s' % matched.group(1))
> > +        # It is necessary to check this because rvaule references
> > +        # parameter packs (c++11 feature)
> > +        if not search(r'&&\.\.\.', line):
> 
> These two ifs could be merged together.

You are right, we could simplify this.

[László Langó]

Created attachment 218037 [details]
Patch

[Peter Gal]

Comment on attachment 218037 [details]
Patch

cool, this looks better now :)

[Zoltan Herczeg]

Comment on attachment 218037 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 218037 [details]
Patch

Clearing flags on attachment: 218037

Committed r160086: <http://trac.webkit.org/changeset/160086>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

Comment on attachment 218037 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=218037&action=review

> Tools/Scripts/webkitpy/style/checkers/cpp.py:1845
> +    # It is necessary to check this, because rvaule references can be in

Typo here: rvaule.