Explicitly delete some Callback constructors to improve errors.

Explicitly delete some Callback constructors to improve errors.

In general, this seems to improve the compile errors by pointing them
directly at the source of the problem, rather than spamming the output
with things like the list of all candidate overloads that didn't match.

Approaches using static_assert() were also considered to improve
readability even further; however, this causes type traits like
std::is_constructible, std::is_assignable, and  std::is_convertible to
incorrectly return true when it should return false.

On clang:
- the error for copying OnceCallback now points directly at the
  deleted copy constructor.
- the error for converting OnceCallback to RepeatingCallback now
  points directly at the deleted conversion constructor, rather than
  printing out all the candidates that failed to match.

BUG=675328

[dcheng, 2016-12-18 10:17:05]

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

[commit-bot: I haz the power, 2016-12-18 10:17:14]

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

[dcheng, 2016-12-18 10:18:22]

Description was changed from

==========
Explicitly delete some Callback constructors to improve errors.

In general, this seems to improve the compile errors by pointing them
directly at the source of the problem, rather than spamming the output
with things like the list of all candidate overloads that didn't match.

Approaches using static_assert() were also considered to improve
readability even further; however, this causes type traits like
std::is_constructible and std::is_convertible to start incorrectly
returning true.

=== Before: Copying OnceCallback ===

../../base/bind_unittest.cc:1420:21: error: call to implicitly-deleted copy
constructor of 'base::OnceClosure' (aka 'base::Callback<void (),
base::internal::CopyMode::MoveOnly, base::internal::RepeatMode::Once>')
  base::OnceClosure cb2 = cb;
                    ^     ~~
../../base/callback.h:103:7: note: copy constructor of 'Callback<void (),
base::internal::CopyMode::MoveOnly, base::internal::RepeatMode::Once>' is
implicitly deleted because base class 'internal::CallbackBase<(CopyMode)0>' has
a deleted copy constructor
    : public internal::CallbackBase<copy_mode>,
      ^
../../base/callback_internal.h:79:3: note: copy constructor is implicitly
deleted because 'CallbackBase<base::internal::CopyMode::MoveOnly>' has a
user-declared move constructor
  CallbackBase(CallbackBase&& c);
  ^
1 error generated.

=== After: Copying OnceCallback ===

../../base/bind_unittest.cc:1420:21: error: call to implicitly-deleted copy
constructor of 'base::OnceClosure' (aka 'base::Callback<void (),
base::internal::CopyMode::MoveOnly, base::internal::RepeatMode::Once>')
  base::OnceClosure cb2 = cb;
                    ^     ~~
../../base/callback.h:103:7: note: copy constructor of 'Callback<void (),
base::internal::CopyMode::MoveOnly, base::internal::RepeatMode::Once>' is
implicitly deleted because base class 'internal::CallbackBase<(CopyMode)0>' has
a deleted copy constructor
    : public internal::CallbackBase<copy_mode>,
      ^
../../base/callback_internal.h:82:3: note: 'CallbackBase' has been explicitly
marked deleted here
  CallbackBase(const CallbackBase&) = delete;
  ^
1 error generated.

=== Before: Converting OnceCallback to RepeatingCallback ===

../../base/bind_unittest.cc:1420:17: error: no viable conversion from
'Callback<[...], internal::CopyMode::MoveOnly aka 0, internal::RepeatMode::Once
aka 0>' to 'Callback<[...], (default) internal::CopyMode::Copyable aka 1,
(default) internal::RepeatMode::Repeating aka 1>'
  base::Closure cb2 = cb;
                ^     ~~
../../base/callback_forward.h:29:7: note: candidate constructor (the implicit
move constructor) not viable: no known conversion from 'base::OnceClosure' (aka
'base::Callback<void (), base::internal::CopyMode::MoveOnly,
base::internal::RepeatMode::Once>') to 'base::Callback<void (),
base::internal::CopyMode::Copyable, base::internal::RepeatMode::Repeating> &&'
for 1st argument
class Callback;
      ^
../../base/callback.h:102:7: note: candidate constructor (the implicit copy
constructor) not viable: no known conversion from 'base::OnceClosure' (aka
'base::Callback<void (), base::internal::CopyMode::MoveOnly,
base::internal::RepeatMode::Once>') to 'const base::Callback<void (),
base::internal::CopyMode::Copyable, base::internal::RepeatMode::Repeating> &'
for 1st argument
class Callback<R(Args...), copy_mode, repeat_mode>
      ^
../../base/callback.h:120:17: note: candidate template ignored: disabled by
'enable_if' [with OtherCallback = base::Callback<void (),
base::internal::CopyMode::MoveOnly, base::internal::RepeatMode::Once>]
                internal::IsCallbackConvertible<OtherCallback, Callback>::value
                ^
1 error generated.

=== After: Converting OnceCallback to RepeatingCallback ===

../../base/bind_unittest.cc:1420:17: error: conversion function from
'Callback<[...], internal::CopyMode::MoveOnly aka 0, internal::RepeatMode::Once
aka 0>' to 'Callback<[...], (default) internal::CopyMode::Copyable aka 1,
(default) internal::RepeatMode::Repeating aka 1>' invokes a deleted function
  base::Closure cb2 = cb;
                ^     ~~
../../base/callback.h:121:3: note: 'Callback<base::Callback<void (),
base::internal::CopyMode::MoveOnly, base::internal::RepeatMode::Once>, void>'
has been explicitly marked deleted here
  Callback(const OtherCallback&) = delete;
  ^
1 error generated.

BUG=675328
==========

to

==========
Explicitly delete some Callback constructors to improve errors.

In general, this seems to improve the compile errors by pointing them
directly at the source of the problem, rather than spamming the output
with things like the list of all candidate overloads that didn't match.

Approaches using static_assert() were also considered to improve
readability even further; however, this causes type traits like
std::is_constructible, std::is_assignable, and  std::is_convertible to
incorrectly return true when it should return false.

=== Before: Copying OnceCallback ===

../../base/bind_unittest.cc:1420:21: error: call to implicitly-deleted copy
constructor of 'base::OnceClosure' (aka 'base::Callback<void (),
base::internal::CopyMode::MoveOnly, base::internal::RepeatMode::Once>')
  base::OnceClosure cb2 = cb;
                    ^     ~~
../../base/callback.h:103:7: note: copy constructor of 'Callback<void (),
base::internal::CopyMode::MoveOnly, base::internal::RepeatMode::Once>' is
implicitly deleted because base class 'internal::CallbackBase<(CopyMode)0>' has
a deleted copy constructor
    : public internal::CallbackBase<copy_mode>,
      ^
../../base/callback_internal.h:79:3: note: copy constructor is implicitly
deleted because 'CallbackBase<base::internal::CopyMode::MoveOnly>' has a
user-declared move constructor
  CallbackBase(CallbackBase&& c);
  ^
1 error generated.

=== After: Copying OnceCallback ===

../../base/bind_unittest.cc:1420:21: error: call to implicitly-deleted copy
constructor of 'base::OnceClosure' (aka 'base::Callback<void (),
base::internal::CopyMode::MoveOnly, base::internal::RepeatMode::Once>')
  base::OnceClosure cb2 = cb;
                    ^     ~~
../../base/callback.h:103:7: note: copy constructor of 'Callback<void (),
base::internal::CopyMode::MoveOnly, base::internal::RepeatMode::Once>' is
implicitly deleted because base class 'internal::CallbackBase<(CopyMode)0>' has
a deleted copy constructor
    : public internal::CallbackBase<copy_mode>,
      ^
../../base/callback_internal.h:82:3: note: 'CallbackBase' has been explicitly
marked deleted here
  CallbackBase(const CallbackBase&) = delete;
  ^
1 error generated.

=== Before: Converting OnceCallback to RepeatingCallback ===

../../base/bind_unittest.cc:1420:17: error: no viable conversion from
'Callback<[...], internal::CopyMode::MoveOnly aka 0, internal::RepeatMode::Once
aka 0>' to 'Callback<[...], (default) internal::CopyMode::Copyable aka 1,
(default) internal::RepeatMode::Repeating aka 1>'
  base::Closure cb2 = cb;
                ^     ~~
../../base/callback_forward.h:29:7: note: candidate constructor (the implicit
move constructor) not viable: no known conversion from 'base::OnceClosure' (aka
'base::Callback<void (), base::internal::CopyMode::MoveOnly,
base::internal::RepeatMode::Once>') to 'base::Callback<void (),
base::internal::CopyMode::Copyable, base::internal::RepeatMode::Repeating> &&'
for 1st argument
class Callback;
      ^
../../base/callback.h:102:7: note: candidate constructor (the implicit copy
constructor) not viable: no known conversion from 'base::OnceClosure' (aka
'base::Callback<void (), base::internal::CopyMode::MoveOnly,
base::internal::RepeatMode::Once>') to 'const base::Callback<void (),
base::internal::CopyMode::Copyable, base::internal::RepeatMode::Repeating> &'
for 1st argument
class Callback<R(Args...), copy_mode, repeat_mode>
      ^
../../base/callback.h:120:17: note: candidate template ignored: disabled by
'enable_if' [with OtherCallback = base::Callback<void (),
base::internal::CopyMode::MoveOnly, base::internal::RepeatMode::Once>]
                internal::IsCallbackConvertible<OtherCallback, Callback>::value
                ^
1 error generated.

=== After: Converting OnceCallback to RepeatingCallback ===

../../base/bind_unittest.cc:1420:17: error: conversion function from
'Callback<[...], internal::CopyMode::MoveOnly aka 0, internal::RepeatMode::Once
aka 0>' to 'Callback<[...], (default) internal::CopyMode::Copyable aka 1,
(default) internal::RepeatMode::Repeating aka 1>' invokes a deleted function
  base::Closure cb2 = cb;
                ^     ~~
../../base/callback.h:121:3: note: 'Callback<base::Callback<void (),
base::internal::CopyMode::MoveOnly, base::internal::RepeatMode::Once>, void>'
has been explicitly marked deleted here
  Callback(const OtherCallback&) = delete;
  ^
1 error generated.

BUG=675328
==========

[dcheng, 2016-12-18 10:18:22]

dcheng@chromium.org changed reviewers:
+ avi@chromium.org, thakis@chromium.org

[commit-bot: I haz the power, 2016-12-18 10:59:32]

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

[commit-bot: I haz the power, 2016-12-18 10:59:33]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[dcheng, 2016-12-19 01:24:18]

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

[commit-bot: I haz the power, 2016-12-19 01:24:24]

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, 2016-12-19 03:13:55]

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

[commit-bot: I haz the power, 2016-12-19 03:13:56]

Dry run: This issue passed the CQ dry run.

[dcheng, 2016-12-19 11:07:40]

WDYT? The error for conversions is much better, but the error for copying is
only marginally better (at least it's more direct now).

+brucedawson, any ideas on the windows failure here where is_constructible ends
up evaluating to true for deleted members? I thought this should have been fixed
in VS2015, but maybe there's another connect bug that I missed.

https://codereview.chromium.org/2581943005/diff/20001/base/bind_unittest.cc
File base/bind_unittest.cc (right):

https://codereview.chromium.org/2581943005/diff/20001/base/bind_unittest.cc#n...
base/bind_unittest.cc:1319: #if !defined(COMPILER_MSVC)
I think this might exclude clang-cl too? Should I add a && !defined(__clang__)
to this?

https://codereview.chromium.org/2581943005/diff/20001/base/callback_internal.h
File base/callback_internal.h (right):

https://codereview.chromium.org/2581943005/diff/20001/base/callback_internal....
base/callback_internal.h:83: CallbackBase& operator=(const CallbackBase&) =
delete;
Deleting it in the Callback template helper itself would improve the error even
further, but it's tricky to do this:
http://stackoverflow.com/questions/27073082/conditionally-disabling-a-copy-co...

I'm not even sure it's possible to apply that pattern here, as it defines a
inheritance relation between the partial specialization and the base template...
but the Callback template already inherits from other things.

[brucedawson, 2016-12-19 19:16:00]

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

[brucedawson, 2016-12-19 19:16:00]

I am a huge fan of verbose commit text so if I think your commit text is too
much then it really is. Consider simplifying the before/after to just a couple
of lines, eliding aggressively. You can always put the full before/after in the
bug and mention in the commit text that more details are available there.

Or leave it. Having the information somewhere is definitely good.

https://codereview.chromium.org/2581943005/diff/20001/base/bind_unittest.cc
File base/bind_unittest.cc (right):

https://codereview.chromium.org/2581943005/diff/20001/base/bind_unittest.cc#n...
base/bind_unittest.cc:1319: #if !defined(COMPILER_MSVC)
On 2016/12/19 11:07:40, dcheng wrote:
> I think this might exclude clang-cl too? Should I add a && !defined(__clang__)
> to this?

Easy enough to test with is_clang = true. There's also a one-time script you
have to run to enable the first download of clang (tools\clang\scripts\update.py
?)

https://codereview.chromium.org/2581943005/diff/20001/base/callback.h
File base/callback.h (right):

https://codereview.chromium.org/2581943005/diff/20001/base/callback.h#newcode126
base/callback.h:126: Callback>::value>::type>
The diffs are very confusing here - it took me a while to realize that the
IsCallbackConvertible clause is unchanged. Can you undo the reformat of that so
that the diffs make the actual change clearer?

Although, I still find enable_if very confusing.

https://codereview.chromium.org/2581943005/diff/20001/base/callback_internal.h
File base/callback_internal.h (right):

https://codereview.chromium.org/2581943005/diff/20001/base/callback_internal....
base/callback_internal.h:83: CallbackBase& operator=(const CallbackBase&) =
delete;
Simple is probably better in that case.

[dcheng, 2016-12-19 19:38:16]

Description was changed from

==========
Explicitly delete some Callback constructors to improve errors.

In general, this seems to improve the compile errors by pointing them
directly at the source of the problem, rather than spamming the output
with things like the list of all candidate overloads that didn't match.

Approaches using static_assert() were also considered to improve
readability even further; however, this causes type traits like
std::is_constructible, std::is_assignable, and  std::is_convertible to
incorrectly return true when it should return false.

=== Before: Copying OnceCallback ===

../../base/bind_unittest.cc:1420:21: error: call to implicitly-deleted copy
constructor of 'base::OnceClosure' (aka 'base::Callback<void (),
base::internal::CopyMode::MoveOnly, base::internal::RepeatMode::Once>')
  base::OnceClosure cb2 = cb;
                    ^     ~~
../../base/callback.h:103:7: note: copy constructor of 'Callback<void (),
base::internal::CopyMode::MoveOnly, base::internal::RepeatMode::Once>' is
implicitly deleted because base class 'internal::CallbackBase<(CopyMode)0>' has
a deleted copy constructor
    : public internal::CallbackBase<copy_mode>,
      ^
../../base/callback_internal.h:79:3: note: copy constructor is implicitly
deleted because 'CallbackBase<base::internal::CopyMode::MoveOnly>' has a
user-declared move constructor
  CallbackBase(CallbackBase&& c);
  ^
1 error generated.

=== After: Copying OnceCallback ===

../../base/bind_unittest.cc:1420:21: error: call to implicitly-deleted copy
constructor of 'base::OnceClosure' (aka 'base::Callback<void (),
base::internal::CopyMode::MoveOnly, base::internal::RepeatMode::Once>')
  base::OnceClosure cb2 = cb;
                    ^     ~~
../../base/callback.h:103:7: note: copy constructor of 'Callback<void (),
base::internal::CopyMode::MoveOnly, base::internal::RepeatMode::Once>' is
implicitly deleted because base class 'internal::CallbackBase<(CopyMode)0>' has
a deleted copy constructor
    : public internal::CallbackBase<copy_mode>,
      ^
../../base/callback_internal.h:82:3: note: 'CallbackBase' has been explicitly
marked deleted here
  CallbackBase(const CallbackBase&) = delete;
  ^
1 error generated.

=== Before: Converting OnceCallback to RepeatingCallback ===

../../base/bind_unittest.cc:1420:17: error: no viable conversion from
'Callback<[...], internal::CopyMode::MoveOnly aka 0, internal::RepeatMode::Once
aka 0>' to 'Callback<[...], (default) internal::CopyMode::Copyable aka 1,
(default) internal::RepeatMode::Repeating aka 1>'
  base::Closure cb2 = cb;
                ^     ~~
../../base/callback_forward.h:29:7: note: candidate constructor (the implicit
move constructor) not viable: no known conversion from 'base::OnceClosure' (aka
'base::Callback<void (), base::internal::CopyMode::MoveOnly,
base::internal::RepeatMode::Once>') to 'base::Callback<void (),
base::internal::CopyMode::Copyable, base::internal::RepeatMode::Repeating> &&'
for 1st argument
class Callback;
      ^
../../base/callback.h:102:7: note: candidate constructor (the implicit copy
constructor) not viable: no known conversion from 'base::OnceClosure' (aka
'base::Callback<void (), base::internal::CopyMode::MoveOnly,
base::internal::RepeatMode::Once>') to 'const base::Callback<void (),
base::internal::CopyMode::Copyable, base::internal::RepeatMode::Repeating> &'
for 1st argument
class Callback<R(Args...), copy_mode, repeat_mode>
      ^
../../base/callback.h:120:17: note: candidate template ignored: disabled by
'enable_if' [with OtherCallback = base::Callback<void (),
base::internal::CopyMode::MoveOnly, base::internal::RepeatMode::Once>]
                internal::IsCallbackConvertible<OtherCallback, Callback>::value
                ^
1 error generated.

=== After: Converting OnceCallback to RepeatingCallback ===

../../base/bind_unittest.cc:1420:17: error: conversion function from
'Callback<[...], internal::CopyMode::MoveOnly aka 0, internal::RepeatMode::Once
aka 0>' to 'Callback<[...], (default) internal::CopyMode::Copyable aka 1,
(default) internal::RepeatMode::Repeating aka 1>' invokes a deleted function
  base::Closure cb2 = cb;
                ^     ~~
../../base/callback.h:121:3: note: 'Callback<base::Callback<void (),
base::internal::CopyMode::MoveOnly, base::internal::RepeatMode::Once>, void>'
has been explicitly marked deleted here
  Callback(const OtherCallback&) = delete;
  ^
1 error generated.

BUG=675328
==========

to

==========
Explicitly delete some Callback constructors to improve errors.

In general, this seems to improve the compile errors by pointing them
directly at the source of the problem, rather than spamming the output
with things like the list of all candidate overloads that didn't match.

Approaches using static_assert() were also considered to improve
readability even further; however, this causes type traits like
std::is_constructible, std::is_assignable, and  std::is_convertible to
incorrectly return true when it should return false.

On clang:
- the error for copying OnceCallback now points directly at the
  deleted copy constructor.
- the error for converting OnceCallback to RepeatingCallback now
  points directly at the deleted conversion constructor, rather than
  printing out all the candidates that failed to match.

BUG=675328
==========

[dcheng, 2016-12-19 20:18:11]

https://codereview.chromium.org/2581943005/diff/20001/base/bind_unittest.cc
File base/bind_unittest.cc (right):

https://codereview.chromium.org/2581943005/diff/20001/base/bind_unittest.cc#n...
base/bind_unittest.cc:1319: #if !defined(COMPILER_MSVC)
On 2016/12/19 19:16:00, brucedawson wrote:
> On 2016/12/19 11:07:40, dcheng wrote:
> > I think this might exclude clang-cl too? Should I add a &&
!defined(__clang__)
> > to this?
> 
> Easy enough to test with is_clang = true. There's also a one-time script you
> have to run to enable the first download of clang
(tools\clang\scripts\update.py
> ?)

yeah I have a clang build setup, but I couldn't remote in.
I've checked now that I'm at my desktop. I think checks against
defined(__clang__) are usually discouraged though... thakis@, does this seem OK
to you? Or would you prefer we lose this check altogether?

Also, is there a good way to search connect to see if this issue is fixed
somewhere upstream? It'd be good to clean this is up if it becomes unnecessary
at some point in the future.

https://codereview.chromium.org/2581943005/diff/20001/base/callback.h
File base/callback.h (right):

https://codereview.chromium.org/2581943005/diff/20001/base/callback.h#newcode126
base/callback.h:126: Callback>::value>::type>
On 2016/12/19 19:16:00, brucedawson wrote:
> The diffs are very confusing here - it took me a while to realize that the
> IsCallbackConvertible clause is unchanged. Can you undo the reformat of that
so
> that the diffs make the actual change clearer?
> 
> Although, I still find enable_if very confusing.

//base requires clang-formatting now =P

Do you think adding a comment above the explicitly deleted methods would be
helpful for disambiguating this diff a bit?

[dcheng, 2016-12-19 20:18:16]

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

[commit-bot: I haz the power, 2016-12-19 20:18:49]

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

[brucedawson, 2016-12-19 20:50:03]

> Do you think adding a comment above the explicitly deleted methods would be
> helpful for disambiguating this diff a bit?

I think it is the "git cl format" of the otherwise unchanged lines which causes
the problem. Reverting that (git cl format is a suggestion not a requirement,
especially on lines that haven't changed) part of the change would be best -
assuming that I am correct that that line didn't have any non white-space
changes.

> Also, is there a good way to search connect to see if this issue is fixed

Google search seems to be the best option.

[dcheng, 2016-12-19 20:54:42]

On 2016/12/19 20:50:03, brucedawson wrote:
> > Do you think adding a comment above the explicitly deleted methods would be
> > helpful for disambiguating this diff a bit?
> 
> I think it is the "git cl format" of the otherwise unchanged lines which
causes
> the problem. Reverting that (git cl format is a suggestion not a requirement,
> especially on lines that haven't changed) part of the change would be best -
> assuming that I am correct that that line didn't have any non white-space
> changes.

Unfortunately, that doesn't work. The diff algorithm thinks these are the
changed lines:

@@ -117,6 +117,11 @@ class Callback<R(Args...), copy_mode, repeat_mode>

   template <typename OtherCallback,
             typename = typename std::enable_if<
+                internal::IsOnceCallback<OtherCallback>::value>::type>
+  Callback(const OtherCallback&) = delete;
+
+  template <typename OtherCallback,
+            typename = typename std::enable_if<
                 internal::IsCallbackConvertible<OtherCallback, Callback>::value
             >::type>
   Callback(OtherCallback other)

So git cl format (and the presubmit check) will enforce that both declarations
are reformatted.

> 
> > Also, is there a good way to search connect to see if this issue is fixed
> 
> Google search seems to be the best option.

I've tried and failed: all the issues I've found seem to be fixed =(

[danakj, 2016-12-19 20:56:50]

You could revert the reformating for review then reformat after to land?
Also.. randomly.. could you use COMPILER_GCC or whatever instead of
__clang__? We use that elsewhere like
https://cs.chromium.org/chromium/src/base/compiler_specific.h?rcl=0&l=95

On Mon, Dec 19, 2016 at 3:54 PM, <dcheng@chromium.org> wrote:

> On 2016/12/19 20:50:03, brucedawson wrote:
> > > Do you think adding a comment above the explicitly deleted methods
> would be
> > > helpful for disambiguating this diff a bit?
> >
> > I think it is the "git cl format" of the otherwise unchanged lines which
> causes
> > the problem. Reverting that (git cl format is a suggestion not a
> requirement,
> > especially on lines that haven't changed) part of the change would be
> best -
> > assuming that I am correct that that line didn't have any non white-space
> > changes.
>
> Unfortunately, that doesn't work. The diff algorithm thinks these are the
> changed lines:
>
> @@ -117,6 +117,11 @@ class Callback<R(Args...), copy_mode, repeat_mode>
>
> template <typename OtherCallback,
> typename = typename std::enable_if<
> + internal::IsOnceCallback<OtherCallback>::value>::type>
> + Callback(const OtherCallback&) = delete;
> +
> + template <typename OtherCallback,
> + typename = typename std::enable_if<
> internal::IsCallbackConvertible<OtherCallback, Callback>::value
> >::type>
> Callback(OtherCallback other)
>
> So git cl format (and the presubmit check) will enforce that both
> declarations
> are reformatted.
>
> >
> > > Also, is there a good way to search connect to see if this issue is
> fixed
> >
> > Google search seems to be the best option.
>
> I've tried and failed: all the issues I've found seem to be fixed =(
>
> https://codereview.chromium.org/2581943005/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[dcheng, 2016-12-19 21:00:48]

On 2016/12/19 20:56:50, danakj_OOO_and_slow wrote:
> You could revert the reformating for review then reformat after to land?

While I can see the diff formatting being confusing, I'm honestly not too
inclined to fight the tooling here, especially if I have to tweak things more =P
The final diff is going to have the same problem anyway.

> Also.. randomly.. could you use COMPILER_GCC or whatever instead of
> __clang__? We use that elsewhere like
> https://cs.chromium.org/chromium/src/base/compiler_specific.h?rcl=0&l=95
> 

As far as I can tell, COMPILER_GCC won't be defined for clang-cl.

> On Mon, Dec 19, 2016 at 3:54 PM, <mailto:dcheng@chromium.org> wrote:
> 
> > On 2016/12/19 20:50:03, brucedawson wrote:
> > > > Do you think adding a comment above the explicitly deleted methods
> > would be
> > > > helpful for disambiguating this diff a bit?
> > >
> > > I think it is the "git cl format" of the otherwise unchanged lines which
> > causes
> > > the problem. Reverting that (git cl format is a suggestion not a
> > requirement,
> > > especially on lines that haven't changed) part of the change would be
> > best -
> > > assuming that I am correct that that line didn't have any non white-space
> > > changes.
> >
> > Unfortunately, that doesn't work. The diff algorithm thinks these are the
> > changed lines:
> >
> > @@ -117,6 +117,11 @@ class Callback<R(Args...), copy_mode, repeat_mode>
> >
> > template <typename OtherCallback,
> > typename = typename std::enable_if<
> > + internal::IsOnceCallback<OtherCallback>::value>::type>
> > + Callback(const OtherCallback&) = delete;
> > +
> > + template <typename OtherCallback,
> > + typename = typename std::enable_if<
> > internal::IsCallbackConvertible<OtherCallback, Callback>::value
> > >::type>
> > Callback(OtherCallback other)
> >
> > So git cl format (and the presubmit check) will enforce that both
> > declarations
> > are reformatted.
> >
> > >
> > > > Also, is there a good way to search connect to see if this issue is
> > fixed
> > >
> > > Google search seems to be the best option.
> >
> > I've tried and failed: all the issues I've found seem to be fixed =(
> >
> > https://codereview.chromium.org/2581943005/
> >
> 
> -- 
> You received this message because you are subscribed to the Google Groups
> "Chromium-reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[brucedawson, 2016-12-19 21:14:31]

On 2016/12/19 21:00:48, dcheng wrote:
> On 2016/12/19 20:56:50, danakj_OOO_and_slow wrote:
> > You could revert the reformating for review then reformat after to land?

One option would be to just ignore the confusing diff - maybe nobody will ever
look at it.

In general however I sometimes get confused when a CL contains semantic and
non-semantic differences, since it takes extra effort to distinguish them. Doing
the white-space changes from git cl format to otherwise unchanged lines (that is
what's happening, right?) would ideally be done in a prior or subsequent CL to
avoid this confusion.

But anyway, lgtm.

[dcheng, 2016-12-19 21:27:45]

On 2016/12/19 21:14:31, brucedawson wrote:
> On 2016/12/19 21:00:48, dcheng wrote:
> > On 2016/12/19 20:56:50, danakj_OOO_and_slow wrote:
> > > You could revert the reformating for review then reformat after to land?
> 
> One option would be to just ignore the confusing diff - maybe nobody will ever
> look at it.
> 
> In general however I sometimes get confused when a CL contains semantic and
> non-semantic differences, since it takes extra effort to distinguish them.
Doing
> the white-space changes from git cl format to otherwise unchanged lines (that
is
> what's happening, right?) would ideally be done in a prior or subsequent CL to
> avoid this confusion.

Fair enough. Note that in this case, I believe the diff algorithm is going to
select the "incorrect" lines no matter what, since the first two new lines of
the new hunk exactly match the original two lines that were there. The
reformatting just affects whether it selects the rest of the std::enable_if or
not.

> 
> But anyway, lgtm.

[commit-bot: I haz the power, 2016-12-19 22:21:09]

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

[commit-bot: I haz the power, 2016-12-19 22:21:10]

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