Add Sk version of std::exchange

Add skstd version of std::exchange

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2381793004

Committed: https://skia.googlesource.com/skia/+/eb0d91cfa278bceba3ec9a3fdcbc7c47a2ed3a3f

[csmartdalton, 2016-09-29 16:57:48]

Description was changed from

==========
Add Sk version of std::exchange

BUG=skia:
==========

to

==========
Add Sk version of std::exchange

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2381793004
==========

[csmartdalton, 2016-09-29 17:04:21]

csmartdalton@google.com changed reviewers:
+ bsalomon@google.com, bungeman@google.com, mtklein@google.com, reed@google.com

[csmartdalton, 2016-09-29 17:04:21]

I want to use this CL to start a discussion about doing this kind of thing.

std::exchange is C++14 so we can't use it yet in skia. However, it would be nice
to have and its meaning and prupose are standardized.

There is already a precedent for doing this kind of thing with SkTSwap.

I don't know if we will ultimately decide to take this, and if we do, if this is
even the right file for it.

Thoughts?

[mtklein_C, 2016-09-29 17:08:29]

mtklein@chromium.org changed reviewers:
+ mtklein@chromium.org

[mtklein_C, 2016-09-29 17:08:29]

Usually we put these things in namespace skstd, otherwise identical to the
future std:: function we'll have.

[csmartdalton, 2016-09-29 18:06:08]

csmartdalton@google.com changed reviewers:
+ halcanary@google.com

[csmartdalton, 2016-09-29 18:06:09]

Like this, in its own file?

[csmartdalton, 2016-09-29 18:08:36]

Description was changed from

==========
Add Sk version of std::exchange

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2381793004
==========

to

==========
Add skstd version of std::exchange

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2381793004
==========

[mtklein_C, 2016-09-29 18:11:02]

https://codereview.chromium.org/2381793004/diff/40001/src/core/SkExchange.h
File src/core/SkExchange.h (right):

https://codereview.chromium.org/2381793004/diff/40001/src/core/SkExchange.h#n...
src/core/SkExchange.h:14: template <typename T, typename U> inline T exchange(T&
obj, U&& newValue) {
Any idea why the standard does <typename T, typename U=T> ?

Should probably make this static, no?

https://codereview.chromium.org/2381793004/diff/40001/src/core/SkExchange.h#n...
src/core/SkExchange.h:15: T oldValue(obj);
Shouldn't we std::move obj?

https://codereview.chromium.org/2381793004/diff/40001/src/core/SkExchange.h#n...
src/core/SkExchange.h:16: obj = newValue;
and std::forward newValue?

[csmartdalton, 2016-09-29 19:19:22]

BTW, what would you think of making an skstd/ subdirectory for stuff like this?
It looks like there are a few files we could move over.

https://codereview.chromium.org/2381793004/diff/40001/src/core/SkExchange.h
File src/core/SkExchange.h (right):

https://codereview.chromium.org/2381793004/diff/40001/src/core/SkExchange.h#n...
src/core/SkExchange.h:14: template <typename T, typename U> inline T exchange(T&
obj, U&& newValue) {
On 2016/09/29 18:11:02, mtklein_C wrote:
> Any idea why the standard does <typename T, typename U=T> ?

I think it's to avoid ambiguous overloads, e.g.:

exchange(some_ptr, nullptr);

> Should probably make this static, no?

I understood that "static" and "inline" had no effect on template methods.
(Which should raise the question on why I marked it inline but not static :)

Is this suggestion just for stylistic reasons?

https://codereview.chromium.org/2381793004/diff/40001/src/core/SkExchange.h#n...
src/core/SkExchange.h:15: T oldValue(obj);
On 2016/09/29 18:11:02, mtklein_C wrote:
> Shouldn't we std::move obj?

Done.

https://codereview.chromium.org/2381793004/diff/40001/src/core/SkExchange.h#n...
src/core/SkExchange.h:16: obj = newValue;
On 2016/09/29 18:11:02, mtklein_C wrote:
> and std::forward newValue?

Done.

[mtklein_C, 2016-09-29 19:36:39]

Moving things around to make an skstd/ subdirectory sounds like make-work to me.
 What's the benefit?

https://codereview.chromium.org/2381793004/diff/40001/src/core/SkExchange.h
File src/core/SkExchange.h (right):

https://codereview.chromium.org/2381793004/diff/40001/src/core/SkExchange.h#n...
src/core/SkExchange.h:14: template <typename T, typename U> inline T exchange(T&
obj, U&& newValue) {
Generally we make all methods declared in headers static, so that they're not
exported from every compilation unit that they're compiled into.  That's true of
templates too: if a template is marked static, all the concrete instances of
that template will be static.

[csmartdalton, 2016-09-29 19:42:26]

On 2016/09/29 19:36:39, mtklein_C wrote:
> Moving things around to make an skstd/ subdirectory sounds like make-work to
me.
>  What's the benefit?

Just not polluting src/core with new files for every C++14 method we may want to
bring in.  Definitely not a big deal for now.

> 
> https://codereview.chromium.org/2381793004/diff/40001/src/core/SkExchange.h
> File src/core/SkExchange.h (right):
> 
>
https://codereview.chromium.org/2381793004/diff/40001/src/core/SkExchange.h#n...
> src/core/SkExchange.h:14: template <typename T, typename U> inline T
exchange(T&
> obj, U&& newValue) {
> Generally we make all methods declared in headers static, so that they're not
> exported from every compilation unit that they're compiled into.  That's true
of
> templates too: if a template is marked static, all the concrete instances of
> that template will be static.

Oh you're right, template methods can have external linkage. My bad.

[mtklein_C, 2016-09-29 19:46:11]

On 2016/09/29 at 19:42:26, csmartdalton wrote:
> On 2016/09/29 19:36:39, mtklein_C wrote:
> > Moving things around to make an skstd/ subdirectory sounds like make-work to
me.
> >  What's the benefit?
> 
> Just not polluting src/core with new files for every C++14 method we may want
to bring in.  Definitely not a big deal for now.

It's a file here or a file there...


lgtm

[csmartdalton, 2016-09-29 19:51:37]

The CQ bit was checked by csmartdalton@google.com

[commit-bot: I haz the power, 2016-09-29 19:51:42]

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

[reed1, 2016-09-29 20:01:33]

is this a replacement for SkTSwap? Can it be?

[csmartdalton, 2016-09-29 20:09:23]

On 2016/09/29 20:01:33, reed1 wrote:
> is this a replacement for SkTSwap? Can it be?

std::exchange isn't quite the same as std::swap.

Its function is to set a variable and return its old value.

Using it to "swap" would look like this:

b = skstd::exchange(a, b);

[commit-bot: I haz the power, 2016-09-29 20:11:25]

Description was changed from

==========
Add skstd version of std::exchange

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2381793004
==========

to

==========
Add skstd version of std::exchange

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2381793004

Committed:
https://skia.googlesource.com/skia/+/eb0d91cfa278bceba3ec9a3fdcbc7c47a2ed3a3f
==========

[commit-bot: I haz the power, 2016-09-29 20:11:27]

Committed patchset #4 (id:60001) as
https://skia.googlesource.com/skia/+/eb0d91cfa278bceba3ec9a3fdcbc7c47a2ed3a3f

[csmartdalton, 2016-09-29 20:17:24]

On 2016/09/29 20:09:23, csmartdalton wrote:
> On 2016/09/29 20:01:33, reed1 wrote:
> > is this a replacement for SkTSwap? Can it be?
> 
> std::exchange isn't quite the same as std::swap.
> 
> Its function is to set a variable and return its old value.
> 
> Using it to "swap" would look like this:
> 
> b = skstd::exchange(a, b);

We could replace SkTSwap with std::swap. SkString is the only class I see that
specializes it, and there we could specialize sts::swap instead.