[webkit-reviews] review granted: [Bug 216447] Replace formatForDebugger() which uses raw char* with debugDescription() : [Attachment 408617] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Simon Fraser (smfr)
<simon.fraser at apple.com>'s request for review:
Bug 216447: Replace formatForDebugger() which uses raw char* with
debugDescription()
https://bugs.webkit.org/show_bug.cgi?id=216447

Attachment 408617: Patch

https://bugs.webkit.org/attachment.cgi?id=408617&action=review




--- Comment #2 from Darin Adler <darin at apple.com> ---
Comment on attachment 408617
  --> https://bugs.webkit.org/attachment.cgi?id=408617
Patch

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

Looks great!

> Source/WebCore/dom/Node.cpp:1767
>      StringBuilder builder;
> -    builder.append(nodeName(), " 0x"_s,
hex(reinterpret_cast<uintptr_t>(this), Lowercase));
> +
> +    auto name = nodeName();
> +    if (name.isEmpty())
> +	   name = "<none>"_s;
> +
> +    builder.append(name, " 0x"_s, hex(reinterpret_cast<uintptr_t>(this),
Lowercase));
>      return builder.toString();

There’s no need to use a StringBuilder here.

    auto name = nodeName();
    return makeString(name.isEmpty() ? "<none>" : "", name, " 0x"_s,
hex(reinterpret_cast<uintptr_t>(this), Lowercase));

> Source/WebCore/dom/Position.cpp:1395
>      StringBuilder result;
>  
>      if (isNull())
>	   result.appendLiteral("<null>");
>      else {
> -	   char s[1024];
>	   result.appendLiteral("offset ");
>	   result.appendNumber(m_offset);
>	   result.appendLiteral(" of ");
> -	   deprecatedNode()->formatForDebugger(s, sizeof(s));
> -	   result.append(s);
> +	   result.append(deprecatedNode()->debugDescription());
>      }
>  
> -    strncpy(buffer, result.toString().utf8().data(), length - 1);
> +    return result.toString();

There’s no need to use a StringBuilder here:

    if (isNull())
	return "<null>"_s;
    return makeString("offset ", m_offset, " of ",
deprecatedNode()->debugDescription());

> Source/WebCore/dom/Range.cpp:855
>      StringBuilder result;
>  
> -    const int FormatBufferSize = 1024;
> -    char s[FormatBufferSize];
>      result.appendLiteral("from offset ");
>      result.appendNumber(m_start.offset());
>      result.appendLiteral(" of ");
> -    startContainer().formatForDebugger(s, FormatBufferSize);
> -    result.append(s);
> +    result.append(startContainer().debugDescription());
>      result.appendLiteral(" to offset ");
>      result.appendNumber(m_end.offset());
>      result.appendLiteral(" of ");
> -    endContainer().formatForDebugger(s, FormatBufferSize);
> -    result.append(s);
> +    result.append(endContainer().debugDescription());
>  
> -    strncpy(buffer, result.toString().utf8().data(), length - 1);
> +    return result.toString();

Again:

    return makeString("from offset ", m_start.offset(), " of ",
startContainer().debugDescription(), " to offset ", m_end.offset(), " of ",
endContainer().debugDescription());

> Source/WebCore/editing/VisibleSelection.cpp:691
>      StringBuilder result;
> -    String s;
>  
>      if (isNone()) {
>	   result.appendLiteral("<none>");
>      } else {
> -	   const int FormatBufferSize = 1024;
> -	   char s[FormatBufferSize];
>	   result.appendLiteral("from ");
> -	   start().formatForDebugger(s, FormatBufferSize);
> -	   result.append(s);
> +	   result.append(start().debugDescription());
>	   result.appendLiteral(" to ");
> -	   end().formatForDebugger(s, FormatBufferSize);
> -	   result.append(s);
> +	   result.append(end().debugDescription());
>      }
>  
> -    strncpy(buffer, result.toString().utf8().data(), length - 1);
> +    return result.toString();

One more time:

    if (isNone())
	return "<none>"_s;
    return makeString("from ", start().debugDescription(), " to ",
end().debugDescription());