check-webkit-style should support C++11 rvalue references

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Sergio Villar Senin]

The script currently considers:

T&&

as a mistake due to the missing whitespace before the two ampersands.

[László Langó]

Created attachment 217405 [details]
Patch

[Zan Dobersek]

Comment on attachment 217405 [details]
Patch

This looks OK, but a small test case should be included.

[László Langó]

(In reply to comment #2)
> (From update of attachment 217405 [details])
> This looks OK, but a small test case should be included.

Ok, i'm on it.

[László Langó]

Created attachment 217418 [details]
Patch

[Brent Fulgham]

Comment on attachment 217418 [details]
Patch

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

A good start, but I think your logic in the "check_style" change is flawed. I think you allow rvalue references now at the cost of ignoring boolean expressions crossing multiple lines.  Please add a test for the multiple-line case and fix the logic for identifying rvalue references.

> Tools/Scripts/webkitpy/style/checkers/cpp.py:2626
> +    if cleansed_line.strip().endswith('||') or cleansed_line.strip().endswith(' &&'):

Won't strip() remove all the whitespace?  Will we ever match ' &&'?

> Tools/Scripts/webkitpy/style/checkers/cpp_unittest.py:629
> +        self.assert_lint('T&&', '')

We also need a test that shows that we catch instances of boolean expressions spanning multiple lines.  I believe that your change will break that style check. You can prove me wrong by including a test case!  :-)

[László Langó]

(In reply to comment #5)
> (From update of attachment 217418 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=217418&action=review
> 
> A good start, but I think your logic in the "check_style" change is flawed. I think you allow rvalue references now at the cost of ignoring boolean expressions crossing multiple lines.  Please add a test for the multiple-line case and fix the logic for identifying rvalue references.
> 
> > Tools/Scripts/webkitpy/style/checkers/cpp.py:2626
> > +    if cleansed_line.strip().endswith('||') or cleansed_line.strip().endswith(' &&'):
> 
> Won't strip() remove all the whitespace?  Will we ever match ' &&'?
> 
> > Tools/Scripts/webkitpy/style/checkers/cpp_unittest.py:629
> > +        self.assert_lint('T&&', '')
> 
> We also need a test that shows that we catch instances of boolean expressions spanning multiple lines.  I believe that your change will break that style check. You can prove me wrong by including a test case!  :-)

stip() method only remove the leading end trailing whitespaces. It won't remove the whitespace before '&&'. Just to be sure a i checked it before i uploaded the patch. End already exists a test that check the multiline conditions (last assert in 'test_brace_at_begin_of_line').

[Brent Fulgham]

Comment on attachment 217418 [details]
Patch

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

R=me

>>> Tools/Scripts/webkitpy/style/checkers/cpp.py:2626
>>> +    if cleansed_line.strip().endswith('||') or cleansed_line.strip().endswith(' &&'):
>> 
>> Won't strip() remove all the whitespace?  Will we ever match ' &&'?
> 
> stip() method only remove the leading end trailing whitespaces. It won't remove the whitespace before '&&'. Just to be sure a i checked it before i uploaded the patch. End already exists a test that check the multiline conditions (last assert in 'test_brace_at_begin_of_line').

Great! Thanks for confirming.

[WebKit Commit Bot]

Comment on attachment 217418 [details]
Patch

Clearing flags on attachment: 217418

Committed r159612: <http://trac.webkit.org/changeset/159612>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[László Langó]

*** Bug 73395 has been marked as a duplicate of this bug. ***