Migrate WTF::Vector::append() to ::push_back() in WTF::HexNumber

Migrate WTF::Vector::append() to ::push_back() in WTF::HexNumber

As discussed on platform-architecture-dev [0], there is interest in
normalizing the methods of WTF classes to better align with std classes.
This CL uses template specialization on WTF::HexNumber::appendByteAsHex,
which needs to support classes (like WTF::Vector) that are being
migrated to push_back() while other classes (like WTF::StringBuilder)
are still using an append() method.

There are no functional changes.

[0] https://groups.google.com/a/chromium.org/d/topic/platform-architecture-dev/I7jnz4p1h84/discussion

BUG=662431
Review-Url: https://codereview.chromium.org/2622363005
Cr-Commit-Position: refs/heads/master@{#446345}
Committed: https://chromium.googlesource.com/chromium/src/+/cb3a30a6acb4e3e774f9c0d416bb90c270065364

[pilgrim_google, 2017-01-12 17:04:05]

The CQ bit was checked by pilgrim@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-12 17:04:24]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[pilgrim_google, 2017-01-12 17:04:40]

pilgrim@chromium.org changed reviewers:
+ esprehn@chromium.org, haraken@chromium.org, jsbell@chromium.org,
yutak@chromium.org

[pilgrim_google, 2017-01-12 17:04:41]

This does not currently compile. The idea is sound but I can't get the syntax
right. Help?

[commit-bot: I haz the power, 2017-01-12 17:17:26]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-12 17:17:27]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[pilgrim_google, 2017-01-12 17:31:21]

The CQ bit was checked by pilgrim@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-12 17:32:25]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-12 17:45:23]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-12 17:45:24]

Dry run: Try jobs failed on following builders:
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[pilgrim_google, 2017-01-12 21:08:15]

The CQ bit was checked by pilgrim@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-12 21:08:55]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-12 21:29:57]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-12 21:29:58]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)

[jsbell, 2017-01-12 23:35:26]

https://codereview.chromium.org/2622363005/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/wtf/HexNumber.h (right):

https://codereview.chromium.org/2622363005/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/wtf/HexNumber.h:51: HexConversionMode mode =
Uppercase) {
This should be just `HexConversionMode mode` (i.e. drop `= Uppercase`) -
apparently it inherits that from the specialized template definition. Builds
fine for me with that.

[pilgrim_google, 2017-01-13 00:50:59]

The CQ bit was checked by pilgrim@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-13 00:52:23]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-13 02:17:40]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-13 02:17:40]

Dry run: This issue passed the CQ dry run.

[pilgrim_google, 2017-01-13 02:45:46]

OK, this is ready for review.

[haraken, 2017-01-13 02:49:42]

LGTM

[Yuta Kitamura, 2017-01-13 07:32:09]

I just have an optional suggestion; your patch LGTM as-is.

https://codereview.chromium.org/2622363005/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/wtf/HexNumber.h (right):

https://codereview.chromium.org/2622363005/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/wtf/HexNumber.h:49: inline void
appendByteAsHex<Vector<LChar>>(unsigned char byte,
Is just specializing for Vector<LChar> sufficient? It might be
worth considering to do Vector<T> partial specialization here
(with a templated class with a static member function), but I agree
that doing that could be overly complex.

[esprehn, 2017-01-13 07:43:07]

I'd prefer we made this generic on Vector<T> instead of LChar.

[jsbell, 2017-01-13 17:13:44]

On 2017/01/13 07:43:07, esprehn wrote:
> I'd prefer we made this generic on Vector<T> instead of LChar.

As noted by yutak, this would require converting this to a templated class with
a static member function. Partial specialization of template functions is not
permitted. 

Given that this is temporary (i.e. I think the plan is to rip out the
specialization once everything is converted to push_back?) what do we want the
end state for the code to be - functions or class statics?

[esprehn, 2017-01-13 17:26:44]

On 2017/01/13 at 17:13:44, jsbell wrote:
> On 2017/01/13 07:43:07, esprehn wrote:
> > I'd prefer we made this generic on Vector<T> instead of LChar.
> 
> As noted by yutak, this would require converting this to a templated class
with a static member function. Partial specialization of template functions is
not permitted. 
> 
> Given that this is temporary (i.e. I think the plan is to rip out the
specialization once everything is converted to push_back?) what do we want the
end state for the code to be - functions or class statics?

I don't think we're going to rename StringBuilder::append to push_back.
std::string has append() not push_back().

[jsbell, 2017-01-13 18:01:13]

On 2017/01/13 17:26:44, esprehn wrote:
> On 2017/01/13 at 17:13:44, jsbell wrote:
> > On 2017/01/13 07:43:07, esprehn wrote:
> > > I'd prefer we made this generic on Vector<T> instead of LChar.
> > 
> > As noted by yutak, this would require converting this to a templated class
> with a static member function. Partial specialization of template functions is
> not permitted. 
> > 
> > Given that this is temporary (i.e. I think the plan is to rip out the
> specialization once everything is converted to push_back?) what do we want the
> end state for the code to be - functions or class statics?
> 
> I don't think we're going to rename StringBuilder::append to push_back.
> std::string has append() not push_back().

Ah, thanks.

So... you're leaning towards introducing a HexNumber template class with
statics? 

Looks like there are only ~20 uses of the append*AsHex*() functions, so not too
terrible.

[esprehn, 2017-01-13 22:17:00]

On 2017/01/13 at 18:01:13, jsbell wrote:
> On 2017/01/13 17:26:44, esprehn wrote:
> > On 2017/01/13 at 17:13:44, jsbell wrote:
> > > On 2017/01/13 07:43:07, esprehn wrote:
> > > > I'd prefer we made this generic on Vector<T> instead of LChar.
> > > 
> > > As noted by yutak, this would require converting this to a templated class
> > with a static member function. Partial specialization of template functions
is
> > not permitted. 
> > > 
> > > Given that this is temporary (i.e. I think the plan is to rip out the
> > specialization once everything is converted to push_back?) what do we want
the
> > end state for the code to be - functions or class statics?
> > 
> > I don't think we're going to rename StringBuilder::append to push_back.
> > std::string has append() not push_back().
> 
> Ah, thanks.
> 
> So... you're leaning towards introducing a HexNumber template class with
statics? 
> 
> Looks like there are only ~20 uses of the append*AsHex*() functions, so not
too terrible.

Sure, we hide that inside the function like:

appendAsHex(T&, ...) {
  Adapter<T>:append(...);
}

you can also detect the existence of a function using some template magic, but
it's a bit ugly looking.

[pilgrim_google, 2017-01-19 17:42:01]

The CQ bit was checked by pilgrim@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-19 17:42:19]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[pilgrim_google, 2017-01-19 17:43:21]

please re-review

[jsbell, 2017-01-19 17:52:30]

https://codereview.chromium.org/2622363005/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/wtf/HexNumber.h (right):

https://codereview.chromium.org/2622363005/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/wtf/HexNumber.h:51: Vector<T>& destination,
Partial specialization of functions is forbidden, but partial specialization of
static methods is just fine. Thanks, C++!

https://codereview.chromium.org/2622363005/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/wtf/HexNumber.h:102: using WTF::HexNumber;
Question for the architecture folks: 

Given that this CL is touching every use of this API anyway, do we want to keep
the 'using' or should we require callers to be explicit about the WTF namespace?

[commit-bot: I haz the power, 2017-01-19 19:59:57]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-19 19:59:58]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Yuta Kitamura, 2017-01-20 04:46:33]

https://codereview.chromium.org/2622363005/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/wtf/HexNumber.h (right):

https://codereview.chromium.org/2622363005/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/wtf/HexNumber.h:51: Vector<T>& destination,
On 2017/01/19 17:52:30, jsbell wrote:
> Partial specialization of functions is forbidden, but partial specialization
of
> static methods is just fine. Thanks, C++!

Strictly speaking, this isn't partial specialization, but
function overloading. This can be done in non-member functions,
too.

[esprehn, 2017-01-20 04:52:57]

On 2017/01/20 at 04:46:33, yutak wrote:
>
https://codereview.chromium.org/2622363005/diff/80001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/wtf/HexNumber.h (right):
> 
>
https://codereview.chromium.org/2622363005/diff/80001/third_party/WebKit/Sour...
> third_party/WebKit/Source/wtf/HexNumber.h:51: Vector<T>& destination,
> On 2017/01/19 17:52:30, jsbell wrote:
> > Partial specialization of functions is forbidden, but partial specialization
of
> > static methods is just fine. Thanks, C++!
> 
> Strictly speaking, this isn't partial specialization, but
> function overloading. This can be done in non-member functions,
> too.

Can we be explicit and have Vector<UChar>, Vector<LChar>, etc. here? There
shouldn't be many types of Vector this function needs to handle, we can write
them out.

[jsbell, 2017-01-20 17:43:10]

On 2017/01/20 04:52:57, esprehn wrote:
> Can we be explicit and have Vector<UChar>, Vector<LChar>, etc. here? There
> shouldn't be many types of Vector this function needs to handle, we can write
> them out.

Isn't this the opposite of the guidance in #24?

[pilgrim_google, 2017-01-24 16:27:55]

Please re-review.

[pilgrim_google, 2017-01-24 16:27:55]

The CQ bit was checked by pilgrim@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-24 16:28:02]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-24 16:31:58]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-24 16:31:59]

Dry run: Try jobs failed on following builders:
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Yuta Kitamura, 2017-01-25 05:33:28]

https://codereview.chromium.org/2622363005/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/wtf/HexNumber.h (right):

https://codereview.chromium.org/2622363005/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/wtf/HexNumber.h:49: template static inline void
appendByteAsHex(
If you intend to specialize a function template, you should replace
"template" with "template <>".

(The syntax "template void func();" is for explicit instantiation
 of a function template. In this case it must not have a function
 body.)

[Yuta Kitamura, 2017-01-25 06:02:52]

LGTM except for the comment above.

[pilgrim_google, 2017-01-25 15:05:15]

On 2017/01/25 at 05:33:28, yutak wrote:
>
https://codereview.chromium.org/2622363005/diff/100001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/wtf/HexNumber.h (right):
> 
>
https://codereview.chromium.org/2622363005/diff/100001/third_party/WebKit/Sou...
> third_party/WebKit/Source/wtf/HexNumber.h:49: template static inline void
appendByteAsHex(
> If you intend to specialize a function template, you should replace
> "template" with "template <>".
> 
> (The syntax "template void func();" is for explicit instantiation
>  of a function template. In this case it must not have a function
>  body.)

That gives me a compile-time error:

In file included from ../../third_party/WebKit/Source/wtf/text/WTFString.cpp:28:
../../third_party/WebKit/Source/wtf/HexNumber.h:49:34: error: explicit
specialization of 'appendByteAsHex' in class scope
  template <> static inline void appendByteAsHex(
                                 ^
1 error generated.

[Yuta Kitamura, 2017-01-26 04:58:24]

On 2017/01/25 15:05:15, pilgrim_google wrote:
> On 2017/01/25 at 05:33:28, yutak wrote:
> >
>
https://codereview.chromium.org/2622363005/diff/100001/third_party/WebKit/Sou...
> > File third_party/WebKit/Source/wtf/HexNumber.h (right):
> > 
> >
>
https://codereview.chromium.org/2622363005/diff/100001/third_party/WebKit/Sou...
> > third_party/WebKit/Source/wtf/HexNumber.h:49: template static inline void
> appendByteAsHex(
> > If you intend to specialize a function template, you should replace
> > "template" with "template <>".
> > 
> > (The syntax "template void func();" is for explicit instantiation
> >  of a function template. In this case it must not have a function
> >  body.)
> 
> That gives me a compile-time error:
> 
> In file included from
../../third_party/WebKit/Source/wtf/text/WTFString.cpp:28:
> ../../third_party/WebKit/Source/wtf/HexNumber.h:49:34: error: explicit
> specialization of 'appendByteAsHex' in class scope
>   template <> static inline void appendByteAsHex(
>                                  ^
> 1 error generated.

Ah sorry, in that case, I think you should just remove
"template". That will make the function overloaded,
and during the overload resolution, non-templated functions
will be preferred over templated ones, so that should work
fine.

[pilgrim_google, 2017-01-26 15:02:10]

The CQ bit was checked by pilgrim@chromium.org

[pilgrim_google, 2017-01-26 15:02:10]

The patchset sent to the CQ was uploaded after l-g-t-m from
haraken@chromium.org, yutak@chromium.org
Link to the patchset: https://codereview.chromium.org/2622363005/#ps120001
(title: "remove template, rely on function overload resolution for
Vector-specific methods")

[commit-bot: I haz the power, 2017-01-26 15:02:24]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-26 16:26:24]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1485442930442220,
"parent_rev": "4aa65a956762bfc23d791bf360d60fe944b84506", "commit_rev":
"cb3a30a6acb4e3e774f9c0d416bb90c270065364"}

[commit-bot: I haz the power, 2017-01-26 16:27:01]

Description was changed from

==========
Migrate WTF::Vector::append() to ::push_back() in WTF::HexNumber

As discussed on platform-architecture-dev [0], there is interest in
normalizing the methods of WTF classes to better align with std classes.
This CL uses template specialization on WTF::HexNumber::appendByteAsHex,
which needs to support classes (like WTF::Vector) that are being
migrated to push_back() while other classes (like WTF::StringBuilder)
are still using an append() method.

There are no functional changes.

[0]
https://groups.google.com/a/chromium.org/d/topic/platform-architecture-dev/I7...

BUG=662431
==========

to

==========
Migrate WTF::Vector::append() to ::push_back() in WTF::HexNumber

As discussed on platform-architecture-dev [0], there is interest in
normalizing the methods of WTF classes to better align with std classes.
This CL uses template specialization on WTF::HexNumber::appendByteAsHex,
which needs to support classes (like WTF::Vector) that are being
migrated to push_back() while other classes (like WTF::StringBuilder)
are still using an append() method.

There are no functional changes.

[0]
https://groups.google.com/a/chromium.org/d/topic/platform-architecture-dev/I7...

BUG=662431

Review-Url: https://codereview.chromium.org/2622363005
Cr-Commit-Position: refs/heads/master@{#446345}
Committed:
https://chromium.googlesource.com/chromium/src/+/cb3a30a6acb4e3e774f9c0d416bb...
==========

[commit-bot: I haz the power, 2017-01-26 16:27:04]

Committed patchset #7 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/cb3a30a6acb4e3e774f9c0d416bb...