DCHECK_EQ with enum value

[Thiago Farina]

Hi folks,

I'm wondering what is the preferred/correct way to convert a
DCHECK(foo == BLAH) into DCHECK_EQ:

Using DIALOGBUTTON_OK as example:

1 - DCHECK_EQ(button, DIALOGBUTTON_OK);
2 - DCHECK_EQ(DIALOGBUTTON_OK, foo)

1 or 2?

Thanks!

[Daniel Cheng]

The general convention (at least in unit tests) is that the expected value is on the left, so I'd expect to follow a similar pattern for [D]CHECK macros.

Daniel

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

[Peter Kasting]

On Thu, Nov 3, 2011 at 7:15 PM, Daniel Cheng <dch...@chromium.org> wrote:

The general convention (at least in unit tests) is that the expected value is on the left, so I'd expect to follow a similar pattern for [D]CHECK macros.

Right -- there's no formal rule, but for DCHECK_EQ and DCHECK_NE, I generally ask in my reviews that people put it as (EXPECTED, ACTUAL) for consistency with the gtest macros.

For _GE/GT/LE/LT, things should just be written "naturally", i.e. "x > 3" -> DCHECK_GT(x, 3), because doing the reverse makes things too hard to understand.

PK 

[Chad Faragher]

On Fri, Nov 4, 2011 at 12:25 PM, Peter Kasting <pkas...@chromium.org> wrote:

Right -- there's no formal rule, but for DCHECK_EQ and DCHECK_NE, I generally ask in my reviews that people put it as (EXPECTED, ACTUAL) for consistency with the gtest macros.

For _GE/GT/LE/LT, things should just be written "naturally", i.e. "x > 3" -> DCHECK_GT(x, 3), because doing the reverse makes things too hard to understand.  [CITATION NEEDED]

 

I just had the opposite experience.  I knew about the order of EXPECTED, ACTUAL, and was originally doing an equality comparison, verifying that some value was equal to a constant and had this check:

  EXPECT_EQ(3, value);

Then I realized that it wasn't that the value needed to be equal to the constant, it needed to be less than or equal, so I just changed the EQ to LE and wound up with:

  EXPECT_LE(3, value); 

Then a huge mystery developed while I tried to figure out why things were failing.  (And it's due to the order of expected and actual being reversed).  

So I reject your claim that it's too hard to understand in reverse.

I also think that if expectations were extensions, then I would have no problem reading EXPECT_LE(3, value) as value.EXPECT_LE(3), and since 'this' arguments tend to the the first logical argument, I would argue that ACTUAL, EXPECTED would be a better convention.  Not that I'm trying to rock the boat, I'm just saying that I don't support your claim that the reverse order is hard to understand.  I DO agree that it doesn't translate to infix operator notation nicely.

- Chad

[Peter Kasting]

On Fri, Nov 4, 2011 at 1:08 PM, Chad Faragher <wy...@chromium.org> wrote:

On Fri, Nov 4, 2011 at 12:25 PM, Peter Kasting <pkas...@chromium.org> wrote:

For _GE/GT/LE/LT, things should just be written "naturally", i.e. "x > 3" -> DCHECK_GT(x, 3), because doing the reverse makes things too hard to understand.  [CITATION NEEDED]

 

I just had the opposite experience.  I knew about the order of EXPECTED, ACTUAL, and was originally doing an equality comparison, verifying that some value was equal to a constant and had this check:

  EXPECT_EQ(3, value);

Then I realized that it wasn't that the value needed to be equal to the constant, it needed to be less than or equal, so I just changed the EQ to LE and wound up with:

  EXPECT_LE(3, value); 

Then a huge mystery developed while I tried to figure out why things were failing.  (And it's due to the order of expected and actual being reversed).  

So I reject your claim that it's too hard to understand in reverse.

I'm not arguing about a hypothetical world in which DCHECK_LE and EXPECT_LE (and friends) did the comparison in reverse like you thought would happen when you first made your change.  I'm arguing that, given the actual implementation where DCHECK_LE(a, b) means "a <= b", the correct reversed form, which involves reversing both the arguments AND the conditional, is harder to understand.

In other words, DCHECK_LE(3, value) actually means "value >= 3" and I contend that if you are writing code and want to assert that value >= 3, "DCHECK_GE(value, 3)" is more natural.

If your case is relevant at all I think it supports my position.

PK