New issue
Advanced search Search tips

Issue 708782 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Apr 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

check-webkit-style not handling multi line strings

Project Member Reported by esprehn@chromium.org, Apr 5 2017

Issue description

ex.
  third_party/WebKit/Source/core/dom/SelectorQueryTest.cpp:65:  Multi-line string ("...") found.  This lint script doesn't do well with such strings, and may give bogus warnings.  They're ugly and unnecessary, and you should use concatenation instead".  [readability/multiline_string] [5]
third_party/WebKit/Source/core/dom/SelectorQueryTest.cpp:70:  Multi-line string ("...") found.  This lint script doesn't do well with such strings, and may give bogus warnings.  They're ugly and unnecessary, and you should use concatenation instead".  [readability/multiline_string] [5]

but somehow this used to work:
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/modules/webaudio/AudioWorkletGlobalScopeTest.cpp?type=cs&q=%22R%5C%22%22+lang:cpp+file:webkit&l=119

since it's used all over the place inside blink.
 
Cc: danakj@chromium.org
That's from cpplint as far as I can tell. Did someone enable that for blink recently? We probably should turn off that check if the check itself says it doesn't handle it well?
+1 let's disable readability/multiline_string for blink then.

(or fixing cpplint is also possible of course)
Let's have globally consistent cpplint settings :-) Raw string literals aren't blink-specific.
Wait, the cpplint in depot_tools says:

 if (line.count('"') - line.count('\\"')) % 2:
    error(filename, linenum, 'readability/multiline_string', 5,
          'Multi-line string ("...") found.  This lint script doesn\'t '
          'do well with such strings, and may give bogus warnings.  '
          'Use C++11 raw strings or concatenation instead.')

which is a slightly different text. So either blink has a second copy of cpplint somewhere, or esprehn's depot_tools is old.
Owner: thakis@chromium.org
Status: Assigned (was: Untriaged)
Yup, this is from https://cs.chromium.org/chromium/src/third_party/WebKit/Tools/Scripts/webkitpy/style/checkers/cpp.py?type=cs&q=multiline_string&l=1029 which looks very, uh, inspired by cpplint's code, but is different. For now, let's fix that, but after the great renaming we can hopefully get rid of the cpp checker in webkitpy.
Status: Started (was: Assigned)
https://codereview.chromium.org/2800883004/
Project Member

Comment 7 by bugdroid1@chromium.org, Apr 6 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/c21b004df95e3c751bcef1d8bfc0a793140d9410

commit c21b004df95e3c751bcef1d8bfc0a793140d9410
Author: thakis <thakis@chromium.org>
Date: Thu Apr 06 02:39:29 2017

Port cpplint's C++11 raw string support to blink's cpp style checker.

Ports the raw string bits from
https://github.com/cpplint/cpplint/commit/2aa5998d820bba053c214543c05a739b9b496fa3
Also do the gratutious style rejiggering that webkitpy has done to the original
(four-space indent, line_number instead of linenum, etc.)

BUG= 708782 

Review-Url: https://codereview.chromium.org/2800883004
Cr-Commit-Position: refs/heads/master@{#462332}

[modify] https://crrev.com/c21b004df95e3c751bcef1d8bfc0a793140d9410/third_party/WebKit/Tools/Scripts/webkitpy/style/checkers/cpp.py
[modify] https://crrev.com/c21b004df95e3c751bcef1d8bfc0a793140d9410/third_party/WebKit/Tools/Scripts/webkitpy/style/checkers/cpp_unittest.py

Status: Fixed (was: Started)
Should be better now.
Thanks thakis

Sign in to add a comment