Remove DCHECK_IMPLIES and CHECK_IMPLIES

Remove DCHECK_IMPLIES and CHECK_IMPLIES
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

[vmpstr, 2015-06-29 18:56:54]

vmpstr@chromium.org changed reviewers:
+ danakj@chromium.org, thakis@chromium.org

[vmpstr, 2015-06-29 18:57:19]

Please take an initial look. I'll fix up the description when we agree that this
is the best way forward.

[sunnyps, 2015-06-29 19:15:47]

On 2015/06/29 at 18:57:19, vmpstr wrote:
> Please take an initial look. I'll fix up the description when we agree that
this is the best way forward.

Why are we removing DCHECK_IMPLIES?

[jdduke (slow), 2015-06-29 19:19:37]

On 2015/06/29 19:15:47, sunnyps wrote:
> On 2015/06/29 at 18:57:19, vmpstr wrote:
> > Please take an initial look. I'll fix up the description when we agree that
> this is the best way forward.
> 
> Why are we removing DCHECK_IMPLIES?

Can we at least replace it with a version that gives meaningful debug info for
the parameter that fails?

[danakj, 2015-06-29 19:30:59]

I really think the LHS of this patch both expresses author intent better and is
easier to read.

"DCHECK if a then b" versus "DCHECK if !a or b, oh, if !!a then b" which
requires extra cognitive steps and then holding the inverse of the inverse of
the thing in your head while you think about the remainder.

My opinion remains that the burden of these macros on base is much less than
their value, but I'd also like to stop debating about them.

LGTM % Nico if he sees an improvement here.

https://codereview.chromium.org/1213193003/diff/1/cc/input/top_controls_manag...
File cc/input/top_controls_manager.cc (right):

https://codereview.chromium.org/1213193003/diff/1/cc/input/top_controls_manag...
cc/input/top_controls_manager.cc:179: DCHECK((direction != HIDING_CONTROLS) ||
(TopControlsShownRatio() > 0.f));
|| has lower precedence you dont need the ()s right? similar comment throughout
when there isnt another || or && in the statement

https://codereview.chromium.org/1213193003/diff/1/content/common/discardable_...
File content/common/discardable_shared_memory_heap.cc (right):

https://codereview.chromium.org/1213193003/diff/1/content/common/discardable_...
content/common/discardable_shared_memory_heap.cc:152: DCHECK((leftover->length_
<= 1) ||
can you make this == 1? I think that blocks<span->length due to above, so this
will be >= 1 (please double check my thinking :))

https://codereview.chromium.org/1213193003/diff/1/content/common/discardable_...
content/common/discardable_shared_memory_heap.cc:256: DCHECK((extra <= 1) ||
(spans_.find(leftover->start_) == spans_.end()));
can you make this == 1

[danakj, 2015-06-29 19:32:13]

On 2015/06/29 19:15:47, sunnyps wrote:
> On 2015/06/29 at 18:57:19, vmpstr wrote:
> > Please take an initial look. I'll fix up the description when we agree that
> this is the best way forward.
> 
> Why are we removing DCHECK_IMPLIES?

Context: https://codereview.chromium.org/576073007/

[sunnyps, 2015-06-29 21:19:42]

On 2015/06/29 at 19:32:13, danakj wrote:
> On 2015/06/29 19:15:47, sunnyps wrote:
> > On 2015/06/29 at 18:57:19, vmpstr wrote:
> > > Please take an initial look. I'll fix up the description when we agree
that
> > this is the best way forward.
> > 
> > Why are we removing DCHECK_IMPLIES?
> 
> Context: https://codereview.chromium.org/576073007/

FWIW I like using this macro in cases like this: "DCHECK_IMPLIES(settings_flag,
some_condition)" which is really equivalent to "if (settings_flag)
DCHECK(some_condition)"

[vmpstr, 2015-09-09 18:33:44]

I'm closing this due to inactivity.

[sky, 2015-10-21 22:41:52]

On 2015/09/09 18:33:44, vmpstr wrote:
> I'm closing this due to inactivity.

I support removing this. I added my rationale here:
https://codereview.chromium.org/576073007/

[jam, 2015-10-21 22:42:42]

On 2015/09/09 18:33:44, vmpstr wrote:
> I'm closing this due to inactivity.

see more confusion at https://codereview.chromium.org/576073007#msg29

is there a reason why this didn't land? were you waiting on Nico?

[danakj, 2015-10-21 22:46:25]

On Wed, Oct 21, 2015 at 3:42 PM, <jam@chromium.org> wrote:

> On 2015/09/09 18:33:44, vmpstr wrote:
>
>> I'm closing this due to inactivity.
>>
>
> see more confusion at https://codereview.chromium.org/576073007#msg29
>
> is there a reason why this didn't land? were you waiting on Nico?
>

Yes, and there was plenty of data on that CL that suggested it is well used
and liked by developers. You can read back through the comments there.

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[sky, 2015-10-21 22:50:19]

On 2015/10/21 22:46:25, danakj wrote:
> On Wed, Oct 21, 2015 at 3:42 PM, <mailto:jam@chromium.org> wrote:
> 
> > On 2015/09/09 18:33:44, vmpstr wrote:
> >
> >> I'm closing this due to inactivity.
> >>
> >
> > see more confusion at https://codereview.chromium.org/576073007#msg29
> >
> > is there a reason why this didn't land? were you waiting on Nico?
> >
> 
> Yes, and there was plenty of data on that CL that suggested it is well used
> and liked by developers. You can read back through the comments there.

Darin happened by our office so I put him on the spot and asked what he thought
DCHECK_IMPLIED would do, and told him it takes two arguments. He hadn't a clue
what it meant either.

That some people use this is not an indication that it should be here. For the
reasons I outlined in the other review I feel this doesn't meet the bar for code
we want in such a common place and should be nuked.

[danakj, 2015-10-21 22:53:36]

On Wed, Oct 21, 2015 at 3:50 PM, <sky@chromium.org> wrote:

> On 2015/10/21 22:46:25, danakj wrote:
>
>> On Wed, Oct 21, 2015 at 3:42 PM, <mailto:jam@chromium.org> wrote:
>>
>
> > On 2015/09/09 18:33:44, vmpstr wrote:
>> >
>> >> I'm closing this due to inactivity.
>> >>
>> >
>> > see more confusion at https://codereview.chromium.org/576073007#msg29
>> >
>> > is there a reason why this didn't land? were you waiting on Nico?
>> >
>>
>
> Yes, and there was plenty of data on that CL that suggested it is well used
>> and liked by developers. You can read back through the comments there.
>>
>
> Darin happened by our office so I put him on the spot and asked what he
> thought
> DCHECK_IMPLIED would do, and told him it takes two arguments. He hadn't a
> clue
> what it meant either.
>
> That some people use this is not an indication that it should be here. For
> the
> reasons I outlined in the other review I feel this doesn't meet the bar
> for code
> we want in such a common place and should be nuked.
>

Let's keep conversation in a single thread? This is going to be confusing.

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.