New issue
Advanced search Search tips
Note: Color blocks (like █ or █) mean that a user may not be available. Tooltip shows the reason.

Issue 684610 link

Starred by 1 user
Status: WontFix
Owner: ----
Closed: Feb 2017
Cc:
danakj@chromium.org
lukasza@chromium.org
dcheng@chromium.org
█ nasko@chromium.org
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug

Blocking:
issue 578344



Sign in to add a comment

isASCIISpace is not renamed in some locations

Project Member Reported by lukasza@chromium.org, Jan 24 2017 
isASCIISpace (and IsASCIIAlpha, IsASCIIDigit, etc.) is renamed in some locations but not in others.


Example compile error:

../../third_party/WebKit/Source/core/frame/csp/MediaListDirective.cpp:42:22: error: use of undeclared identifier 'isASCIISpace'; did you mean 'IsASCIISpace'?
    skipWhile<UChar, isASCIISpace>(position, end);
                     ^~~~~~~~~~~~
                     IsASCIISpace
../../third_party/WebKit/Source/wtf/ASCIICType.h:175:12: note: 'IsASCIISpace' declared here
using WTF::IsASCIISpace;
           ^


Example where isASCIISpace got renamed - platform/network/HTTPParsers.cpp:
    bool ParseSuboriginHeader(const String& header,
                              Suborigin* suborigin,
                              WTF::Vector<String>& messages) {
    ...
      Vector<UChar> characters;
      headers[0].AppendTo(characters);

      const UChar* position = characters.Data();
      const UChar* end = position + characters.Size();

      skipWhile<UChar, IsASCIISpace>(position, end);



Example where isASCIISpace did not get renamed - core/frame/csp/MediaListDirective.cpp:

    void MediaListDirective::Parse(const UChar* begin, const UChar* end) {
    ...
      while (position < end) {
        // _____ OR _____mime1/mime1
        // ^        ^
        skipWhile<UChar, isASCIISpace>(position, end);

Comment 1 by lukasza@chromium.org, Jan 24 2017 

AST dump corresponding to the skipWhile call in MediaListDirective::Parse, where isASCIISpace did not get rewritten:

  |       |-CallExpr 0x7f35d92113a0 <line:42:5, col:49> 'void'
  |       | |-ImplicitCastExpr 0x7f35d9211388 <col:5, col:34> 'void (*)(const unsigned short *&, const unsigned short *)' <FunctionToPointerDecay>
  |       | | `-DeclRefExpr 0x7f35d9211288 <col:5, col:34> 'void (const unsigned short *&, const unsigned short *)' lvalue Function 0x7f35d9211170 'skipWhile' 'void (const unsigned short *&, const unsigned short *)' (FunctionTemplate 0x7f35d91fc068 'skipWhile')
  |       | |-DeclRefExpr 0x7f35d9210b48 <col:36> 'const UChar *' lvalue Var 0x7f35d920d058 'position' 'const UChar *'
  |       | `-ImplicitCastExpr 0x7f35d92113d8 <col:46> 'const UChar *' <LValueToRValue>
  |       |   `-DeclRefExpr 0x7f35d9210b70 <col:46> 'const UChar *' lvalue ParmVar 0x7f35d920cec8 'end' 'const UChar *'


AST dump corresponding to the skipWhile call in ParseSuboriginHeader, where isASCIISpace got rewritten:

  | | |-CallExpr 0x8dfcef0 <line:786:3, col:47> 'void'
  | | | |-ImplicitCastExpr 0x8dfced8 <col:3, col:32> 'void (*)(const unsigned short *&, const unsigned short *)' <Func
tionToPointerDecay>
  | | | | `-DeclRefExpr 0x8dfce38 <col:3, col:32> 'void (const unsigned short *&, const unsigned short *)' lvalue Function 0x8dfcd20 'skipWhile' 'void (const unsigned short *&, const unsigned short *)' (FunctionTemplate 0x8d7b3d8 'skipWhile')
  | | | |-DeclRefExpr 0x8dfc958 <col:34> 'const UChar *' lvalue Var 0x8dfc4e8 'position' 'const UChar *'
  | | | `-ImplicitCastExpr 0x8dfcf28 <col:44> 'const UChar *' <LValueToRValue>
  | | |   `-DeclRefExpr 0x8dfc980 <col:44> 'const UChar *' lvalue Var 0x8dfc680 'end' 'const UChar *'

Comment 2 by lukasza@chromium.org, Jan 28 2017 

FWIW, I think we can just use sed to fixup things after the rename:

find third_party/WebKit/Source -type f \
    | xargs grep -l '\bisASCII' \
    | xargs -n 1 sed -i -e 's/\bisASCII/IsASCII/g'
git commit -a -m "Renaming isASCII* function templates to IsASCII*"

I've added the above to the step-by-step doc.

Comment 3 by nasko@chromium.org, Feb 1 2017 

I've used a bit safer regex for those replacements 's/\<isASCII/\u&/g' which uppercases the first character of the match. It also is easily generalizable for iterating over multiple patterns, which can be generated by a script.

Comment 4 by lukasza@chromium.org, Feb 6 2017

Status: WontFix (was: Untriaged)
This can be quite accurately fixed via "sed" after executing the big rename.  I've added appropriate instructions to the step-by-step document.
►

Sign in to add a comment