On the removal of CHECK_IMPLIES

[Carlos Knippschild]

I noticed the macro CHECK_IMPLIES (and its sister DCHECK_IMPLIES) were silently removed in crbug.com/1421483005. I understand they were not seeing much usage but I blame it on the fact that they have never been properly announced nor explained to devs.

I myself think those macros made CHECKs like this one below significantly more readable:

-    DCHECK_IMPLIES(should_reuse_web_ui_, web_ui_);

+    DCHECK(!should_reuse_web_ui_ || web_ui_);

This is even more true for more complex examples like this one:

-  DCHECK_IMPLIES(base::CommandLine::ForCurrentProcess()->HasSwitch(

-                     switches::kEnableBrowserSideNavigation),

-                 stream_override_.get() ||

-                     request.frameType() == WebURLRequest::FrameTypeNone);

+  DCHECK(!base::CommandLine::ForCurrentProcess()->HasSwitch(

+             switches::kEnableBrowserSideNavigation) ||

+         stream_override_.get() ||

+         request.frameType() == WebURLRequest::FrameTypeNone);

I would really like to have them back...

[Lei Zhang]

Link should have been https://crrev.com/1421483005

> --
> --
> Chromium Developers mailing list: chromi...@chromium.org
> View archives, change email options, or unsubscribe:
> http://groups.google.com/a/chromium.org/group/chromium-dev

[Daniel Cheng]

An alternate way to write those checks is with an explicit if (...). I think that's just as clear as (D?)CHECK_IMPLIES.

Daniel

[Carlos Knippschild]

@Daniel: that has the consequence of potentially adding debug-only code into release builds. We generally tend to avoid that to limit size, improve performance, etc.

@Lei: thanks for fixing my link.

[Nick Carter]

I, too, will miss DCHECK_IMPLIES -- and genuinely struggle to understand why it was so polarizing. When DCHECK_IMPLIES was introduced, I recall thinking, "Cool, I've wanted something like that for a while! It's like if (a) DCHECK(b) except a gets compiled out."

It seemed perfectly intuitive to me; but since the detractors persisted for 13 months, it must have seemed equally unintuitive to them.

It seems doubtful that some of the existing macros, e.g., DPCHECK, would survive if subjected to the same level of scrutiny as DCHECK_IMPLIES. But I'm not going to argue that we should remove DPCHECK -- I don't see how it hurts anybody, and it's easy enough to look up should it appear in a code review.

Having said that, I don't object to the rollback, because the most important thing is for the dispute to be resolved.

 - nick

[Dana Jansens]

For what it's worth, here's the conversation:

https://codereview.chromium.org/1213193003/

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

https://codereview.chromium.org/1421483005

On Thu, Oct 29, 2015 at 9:22 AM, Carlos Knippschild <car...@chromium.org> wrote:

--

[Scott Hess]

Maybe it would be easier to add a solution to wrap debug-only code.  Like

#if DCHECK_IS_ON()

#define DEVAL(x) (x)

#else

#define DEVAL(x) 0

#endif

if (DEVAL(o->TestSomething(x, y, z))

  DCHECK(o->SomethingElseHolds(z));

IMHO the above is horrible, but maybe someone out there is smarter than me.  For this case, whether it returns true or false outside debug mode is not important (both should be optimized away), but returning 0 makes it more general.  I suggest this because something like DEVAL() would be more generally applicable than DCHECK_IMPLIES(), so it potentially could have more usage.

That said, I'd guess that most cases with compound conditions like this will be complicated, and sprinkling sugar around won't help much.  Trying to deal with them risks ending up creating something like gmock, where you can't figure out if the disease was bad enough to warrant the cure.

-scott

[Elliott Sprehn]

This already exists in blink, we have ASSERT_ENABLED and guard with #if.

[Nico Weber]

On Thu, Oct 29, 2015 at 9:32 AM, Carlos Knippschild <car...@chromium.org> wrote:

@Daniel: that has the consequence of potentially adding debug-only code into release builds. We generally tend to avoid that to limit size, improve performance, etc.

  if (a)

    DCHECK(b);

will be optimized out by the compiler just like DCHECK_IMPLIES(a, b) as long as the compiler knows that a has no side effects. You can check this yourself by adding a function like

  void foo(bool should_reuse_web_ui_, bool web_ui_) {

    if (should_reuse_web_ui_)

     DCHECK(web_ui_);

  }

to some file in base_unittests (say, base/rand_util.cc), then doing a release build of base_unittests with `ninja -c out/Release base_unittests -v`, copying the compile line for that .cc file, and rerunning it with "-S -o foo.S" and then looking at the assembly for foo().

If a _has_ side effects, it shouldn't be called from a DCHECK. It's of course possible that a is a function call to a function that does not have side effects but isn't marked as such – in that case, the compiler has to call it. But in the vast majority of cases (see the removal diff), that's not the case – a is usually either a bool or a call to an inline accessor, both of which the compiler can reason about. So I don't think this part is an issue.

See the discussion Dana linked to for the rest of your points.

[Peter Kasting]

On Thu, Oct 29, 2015 at 10:31 AM, Nick Carter <ni...@chromium.org> wrote:

I, too, will miss DCHECK_IMPLIES -- and genuinely struggle to understand why it was so polarizing. When DCHECK_IMPLIES was introduced, I recall thinking, "Cool, I've wanted something like that for a while! It's like if (a) DCHECK(b) except a gets compiled out."

It seemed perfectly intuitive to me; but since the detractors persisted for 13 months, it must have seemed equally unintuitive to them.

As someone encountering it for the first time in this thread, I definitely find CHECK_IMPLIES less intuitive to read than the more-obvious expanded form CHECK(!a || b), in part because my brain doesn't handle reverse-polish notation well and "implies a b" is confusing to me.  It's also a reasonably uncommon case, which suggests the macro is of limited benefit.  It also doesn't handle the case of bi-directional implication (i.e. equivalence), CHECK_EQ(a == b, c == d), which in my experience is both more common and harder to read than the pattern CHECK_IMPLIES solves.

The style guide is strongly biased against macros, and I think adding more syntactic sugar macros around other macros is equivalent to "more macros" -- in this case we'd use a macro either way, but now readers have a larger number of macros to be familiar with, so there's still a loss.

Finally, as Nico notes, there should rarely if ever be any performance concern with

if (a)

  DCHECK(b);

So if that's more readable to author and reviewer than DCHECK(!a || b), feel free to use it and don't worry about it.

For all these reasons, I think the decision to remove was correct.

PK