Bug 205759 – Further simplify StringBuilder usage by standardizing hex formating to a single hex() function

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Sam Weinig]

Further simplify StringBuilder usage by standardizing hex formating to a single hex() function

[Sam Weinig]

Created attachment 386729 [details]
Patch

[Sam Weinig]

Darin, you have been working in this area a bit, any thoughts on this simplification? It applies the mapping:

appendByteAsHex(value, builder, case) 
  -> builder.append(hex(static_cast<unsigned char>(value), 2, case))
appendUnsignedAsHex(value, builder, case) 
  -> builder.append(hex(value, case))
appendUnsignedAsHexFixedSize(value, builder, size, case)
  -> builder.append(hex(value, size, case))

[Darin Adler]

(In reply to Sam Weinig from comment #2)
> appendByteAsHex(value, builder, case) 
>   -> builder.append(hex(static_cast<unsigned char>(value), 2, case))

This seems a bit clumsy. But I suppose only a bit.

> appendUnsignedAsHex(value, builder, case) 
>   -> builder.append(hex(value, case))
> appendUnsignedAsHexFixedSize(value, builder, size, case)
>   -> builder.append(hex(value, size, case))

These seem great to me. However, I don’t think we are being consistent in style. Sometimes we name these things:

    FormattedNumber::fixedWidth

Other times we name something similar:

    hex

We should decide what style we are after.

[Sam Weinig]

(In reply to Darin Adler from comment #3)
> (In reply to Sam Weinig from comment #2)
> > appendByteAsHex(value, builder, case) 
> >   -> builder.append(hex(static_cast<unsigned char>(value), 2, case))
> 
> This seems a bit clumsy. But I suppose only a bit.
> 
> > appendUnsignedAsHex(value, builder, case) 
> >   -> builder.append(hex(value, case))
> > appendUnsignedAsHexFixedSize(value, builder, size, case)
> >   -> builder.append(hex(value, size, case))
> 
> These seem great to me. However, I don’t think we are being consistent in
> style. Sometimes we name these things:
> 
>     FormattedNumber::fixedWidth
> 
> Other times we name something similar:
> 
>     hex
> 
> We should decide what style we are after.

We definitely need to decide on the style. I am leaning toward the shorter, unqualified names (hex(...), pad(...), fixedWidth(...), fixedPrecision(...), etc), and putting them all in a shared header for easier discovery. I could be convinced having some qualifier namespace might be helpful, e.g. all in a Format namespace (Format::hex(...), Format::pad(...), etc).

Do you have a particular preference?

[Sam Weinig]

Created attachment 386768 [details]
Patch

[Sam Weinig]

I will unify the style with fixedWidth/fixedPrecision in a follow up.

[Sam Weinig]

Created attachment 386771 [details]
Patch

[Darin Adler]

I like the short names for clarity at the point of use. I like the long names for searching the entire source tree to see where things are used.

[Dean Jackson]

Comment on attachment 386771 [details]
Patch

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

> Tools/TestWebKitAPI/Tests/WTF/HexNumber.cpp:34
> +// Not using builder.toString() or builder.toStringPreserveCapacity() because they all
> +// change internal state of builder.

Why does that matter? You never use the builder again after the expect() method.

[Sam Weinig]

(In reply to Dean Jackson from comment #9)
> Comment on attachment 386771 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=386771&action=review
> 
> > Tools/TestWebKitAPI/Tests/WTF/HexNumber.cpp:34
> > +// Not using builder.toString() or builder.toStringPreserveCapacity() because they all
> > +// change internal state of builder.
> 
> Why does that matter? You never use the builder again after the expect()
> method.

Oh, that comment was copied from another test. That was silly of me.

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 386771 [details]:

transitions/default-timing-function.html bug 138901 (authors: [e-mail] and [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 386771 [details]
Patch

Clearing flags on attachment: 386771

Committed r254046: <https://trac.webkit.org/changeset/254046>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/58328979>