Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
576073007:
base: Add CHECK_IMPLIES and DCHECK_IMPLIES. (Closed)
Descriptionbase: Add CHECK_IMPLIES and DCHECK_IMPLIES.
This patch adds convenience defines for implies relationship.
a implies b is equivalent to !a || b.
This allows things like the following:
CHECK_IMPLIES(ptr, ptr->is_good);
instead of the more cumbersome:
CHECK(!ptr || ptr->is_good).
Committed: https://crrev.com/0456d13053908dc3929035e4abf8799c6e9627a5
Cr-Commit-Position: refs/heads/master@{#297941}
Patch Set 1 #
Messages
Total messages: 38 (5 generated)
danakj@chromium.org changed reviewers: + ajwong@chromium.org, danakj@chromium.org, willchan@chromium.org
LGTM +willchan/ajwong for OWNERS
On 2014/09/17 18:13:28, danakj wrote: > LGTM > > +willchan/ajwong for OWNERS friendly ping for owner's review.
Haha, bad luck with reviewer choices. Albert just went on leave and I'm going on leave (hence my nick change). I'd change reviewers. Reviewers: willchan OOO until 03-22-15, awong, danakj, Message: On 2014/09/17 18:13:28, danakj wrote: > LGTM > +willchan/ajwong for OWNERS > friendly ping for owner's review. Description: base: Add CHECK_IMPLIES and DCHECK_IMPLIES. This patch adds convenience defines for implies relationship. a implies b is equivalent to !a || b. This allows things like the following: CHECK_IMPLIES(ptr, ptr->is_good); instead of the more cumbersome: CHECK(!ptr || ptr->is_good). Please review this at https://codereview.chromium.org/576073007/ SVN Base: https://chromium.googlesource.com/chromium/src.git@master Affected files (+2, -0 lines): M base/logging.h Index: base/logging.h diff --git a/base/logging.h b/base/logging.h index 743586492eb615de6fd279a38b0ff2d1d524bf34.. 33bfbc76c9b5a4309b67c00e5bdb518541ab680c 100644 --- a/base/logging.h +++ b/base/logging.h @@ -532,6 +532,7 @@ DEFINE_CHECK_OP_IMPL(GT, > ) #define CHECK_LT(val1, val2) CHECK_OP(LT, < , val1, val2) #define CHECK_GE(val1, val2) CHECK_OP(GE, >=, val1, val2) #define CHECK_GT(val1, val2) CHECK_OP(GT, > , val1, val2) +#define CHECK_IMPLIES(val1, val2) CHECK(!(val1) || (val2)) #if defined(NDEBUG) #define ENABLE_DLOG 0 @@ -662,6 +663,7 @@ const LogSeverity LOG_DCHECK = LOG_INFO; #define DCHECK_LT(val1, val2) DCHECK_OP(LT, < , val1, val2) #define DCHECK_GE(val1, val2) DCHECK_OP(GE, >=, val1, val2) #define DCHECK_GT(val1, val2) DCHECK_OP(GT, > , val1, val2) +#define DCHECK_IMPLIES(val1, val2) DCHECK(!(val1) || (val2)) #if defined(NDEBUG) && defined(OS_CHROMEOS) #define NOTREACHED() LOG(ERROR) << "NOTREACHED() hit in " << \ To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
danakj@chromium.org changed reviewers: + thakis@chomium.org - ajwong@chromium.org, willchan@chromium.org
thakis: can you OWNERS review please?
jrobbins@google.com changed reviewers: + thakis@chromium.org - thakis@chomium.org
On 2014/09/18 23:40:16, danakj wrote: > thakis: can you OWNERS review please? ping (although Nico is hiding, according to his username)
Sorry about the delay; this is the first time I see this in my inbox. (Maybe you sent it to thakis@chomium.org, rietveld had an invalid autocomplete entry for me recently) Is this heavily used? As far as I know, people write if (val1) CHECK(val2); instead, which seems more immediately clear.
On Wed, Sep 24, 2014 at 4:55 PM, <thakis@chromium.org> wrote: > Sorry about the delay; this is the first time I see this in my inbox. > (Maybe you > sent it to thakis@chomium.org, rietveld had an invalid autocomplete entry > for me > recently) > > Is this heavily used? As far as I know, people write > > if (val1) > CHECK(val2); > > instead, which seems more immediately clear. > We've discouraged doing that in cc as it leaves code outside of the DCHECK which is only meant to be used with the DCHECK. #if DCHECK_IS_ON if (val1) DCHECK(val2) #endif would be an equivalent very verbose way to do this. Perhaps it makes less sense in the CHECK() case, other than for symmetry and the ability to add/remove the D to change the check's behaviour. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
The CQ bit was checked by vmpstr@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/576073007/1
My first official base owner LGTM. I think this is super great and don't want to see DCHECK macros spawning in random places, with 0 cost to the base/ binary size.
Message was sent while issue was closed.
Committed patchset #1 (id:1) as e980739d7a85e9684736b380748fd39238c2965f
Message was sent while issue was closed.
Patchset 1 (id:??) landed as https://crrev.com/0456d13053908dc3929035e4abf8799c6e9627a5 Cr-Commit-Position: refs/heads/master@{#297941}
Message was sent while issue was closed.
About a month later, this is still used in only 2 places: https://code.google.com/p/chromium/codesearch#search/&q=dcheck_implies&sq=pac... Is it really worth having a dedicated macro for this?
Message was sent while issue was closed.
On Mon, Oct 27, 2014 at 5:30 PM, <thakis@chromium.org> wrote: > About a month later, this is still used in only 2 places: > https://code.google.com/p/chromium/codesearch#search/&q= > dcheck_implies&sq=package:chromium&type=cs Well there's 2 more in here https://codereview.chromium.org/640063010/ We never went through and changed !foo || a to IMPLIES(foo, a) I've only been using it in new code or when changing an existing DCHECK. So I'm not sure if this is a good measure of value at this point. Or maybe it is? Is it really worth having a dedicated macro for this? I still like it and you still don't I guess :) You must see this as having some cost to base. I thought adding this was basically 0 cost. That must be where our opinions differ strongly. Maybe you could explain why not having it would be better? To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Message was sent while issue was closed.
On Mon, Oct 27, 2014 at 3:13 PM, Dana Jansens <danakj@chromium.org> wrote: > On Mon, Oct 27, 2014 at 5:30 PM, <thakis@chromium.org> wrote: > >> About a month later, this is still used in only 2 places: >> https://code.google.com/p/chromium/codesearch#search/&q= >> dcheck_implies&sq=package:chromium&type=cs > > > Well there's 2 more in here https://codereview.chromium.org/640063010/ > Also in cc :-) > We never went through and changed !foo || a to IMPLIES(foo, a) I've only > been using it in new code or when changing an existing DCHECK. So I'm not > sure if this is a good measure of value at this point. Or maybe it is? > > Is it really worth having a dedicated macro for this? > > > I still like it and you still don't I guess :) > > You must see this as having some cost to base. I thought adding this was > basically 0 cost. That must be where our opinions differ strongly. Maybe > you could explain why not having it would be better? > Sure! The cost is that it's something people need to learn about when reading code. It sounds very similar to me like the last paragraph in https://chromium.googlesource.com/chromium/src/+/3daa03c0977f4314b76411519e57... that we added today. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Message was sent while issue was closed.
(But I'm happy to wait another N months and see if more than 5 people start using it.) On Mon, Oct 27, 2014 at 3:35 PM, Nico Weber <thakis@chromium.org> wrote: > On Mon, Oct 27, 2014 at 3:13 PM, Dana Jansens <danakj@chromium.org> wrote: > >> On Mon, Oct 27, 2014 at 5:30 PM, <thakis@chromium.org> wrote: >> >>> About a month later, this is still used in only 2 places: >>> https://code.google.com/p/chromium/codesearch#search/&q= >>> dcheck_implies&sq=package:chromium&type=cs >> >> >> Well there's 2 more in here https://codereview.chromium.org/640063010/ >> > > Also in cc :-) > > >> We never went through and changed !foo || a to IMPLIES(foo, a) I've only >> been using it in new code or when changing an existing DCHECK. So I'm not >> sure if this is a good measure of value at this point. Or maybe it is? >> >> Is it really worth having a dedicated macro for this? >> >> >> I still like it and you still don't I guess :) >> >> You must see this as having some cost to base. I thought adding this was >> basically 0 cost. That must be where our opinions differ strongly. Maybe >> you could explain why not having it would be better? >> > > Sure! The cost is that it's something people need to learn about when > reading code. It sounds very similar to me like the last paragraph in > https://chromium.googlesource.com/chromium/src/+/3daa03c0977f4314b76411519e57... > that we added today. > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Message was sent while issue was closed.
On Mon, Oct 27, 2014 at 6:35 PM, Nico Weber <thakis@chromium.org> wrote: > (But I'm happy to wait another N months and see if more than 5 people > start using it.) > OK let's snooze and revisit around the new year. > On Mon, Oct 27, 2014 at 3:35 PM, Nico Weber <thakis@chromium.org> wrote: > >> On Mon, Oct 27, 2014 at 3:13 PM, Dana Jansens <danakj@chromium.org> >> wrote: >> >>> On Mon, Oct 27, 2014 at 5:30 PM, <thakis@chromium.org> wrote: >>> >>>> About a month later, this is still used in only 2 places: >>>> https://code.google.com/p/chromium/codesearch#search/&q= >>>> dcheck_implies&sq=package:chromium&type=cs >>> >>> >>> Well there's 2 more in here https://codereview.chromium.org/640063010/ >>> >> >> Also in cc :-) >> >> >>> We never went through and changed !foo || a to IMPLIES(foo, a) I've only >>> been using it in new code or when changing an existing DCHECK. So I'm not >>> sure if this is a good measure of value at this point. Or maybe it is? >>> >>> Is it really worth having a dedicated macro for this? >>> >>> >>> I still like it and you still don't I guess :) >>> >>> You must see this as having some cost to base. I thought adding this was >>> basically 0 cost. That must be where our opinions differ strongly. Maybe >>> you could explain why not having it would be better? >>> >> >> Sure! The cost is that it's something people need to learn about when >> reading code. It sounds very similar to me like the last paragraph in >> https://chromium.googlesource.com/chromium/src/+/3daa03c0977f4314b76411519e57... >> that we added today. >> > I think my opinion is that when this is used (well?), it's faster/easier to go look up what DCHECK_IMPLIES means and read the code in it and understand what it's checking, than it is to pull apart a complicated !a || b and understand what it is checking. I've had some where I got it wrong a few times before I got it right. Over time I've gotten better at this from recognizing it as a pattern, but that took some time, where as IMPLIES(a, b) is quite intuitive once you learn the trivial syntax. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Message was sent while issue was closed.
ping? i just saw this change when Dana asked me to use it. I have similar feelings as Nico about the benefit of adding a new dcheck macro like this..
Message was sent while issue was closed.
On 2015/06/15 20:27:56, jam wrote: > ping? i just saw this change when Dana asked me to use it. I have similar > feelings as Nico about the benefit of adding a new dcheck macro like this.. So there are now 56 uses of DCHECK_IMPLIES, in ui/ gpu/ content/ cc/ and base/. Generally when people see it the response has been positive like "Oh that makes it easier to read" and whatnot. I'd still like to keep this, I feel like it would regress readability of dchecks (for some more than others). Sometimes A||B is what you logically mean. Sometimes !A||B is what you logically mean, and implies is what you're trying to write.
Message was sent while issue was closed.
On 2015/06/15 20:32:30, danakj wrote: > On 2015/06/15 20:27:56, jam wrote: > > ping? i just saw this change when Dana asked me to use it. I have similar > > feelings as Nico about the benefit of adding a new dcheck macro like this.. > > So there are now 56 uses of DCHECK_IMPLIES, in ui/ gpu/ content/ cc/ and base/. For comparison, there are 8K DCHECKs... It seems that 56 uses in 8 months is a very minimal amount of usage. > Generally when people see it the response has been positive like "Oh that makes > it easier to read" and whatnot. I'd still like to keep this, I feel like it > would regress readability of dchecks (for some more than others). Sometimes A||B > is what you logically mean. Sometimes !A||B is what you logically mean, and > implies is what you're trying to write. I think given that almost all other similar usage just uses DCHECK, folks already know how to read it using DCHECK. There's a cognitive overhead of adding all these variants. I don't feel super strongly about this, but IMO having fewer variants is better.
Message was sent while issue was closed.
On 2015/06/16 03:06:48, jam wrote: > On 2015/06/15 20:32:30, danakj wrote: > > On 2015/06/15 20:27:56, jam wrote: > > > ping? i just saw this change when Dana asked me to use it. I have similar > > > feelings as Nico about the benefit of adding a new dcheck macro like this.. > > > > So there are now 56 uses of DCHECK_IMPLIES, in ui/ gpu/ content/ cc/ and > base/. > > For comparison, there are 8K DCHECKs... > > It seems that 56 uses in 8 months is a very minimal amount of usage. > > > Generally when people see it the response has been positive like "Oh that > makes > > it easier to read" and whatnot. I'd still like to keep this, I feel like it > > would regress readability of dchecks (for some more than others). Sometimes > A||B > > is what you logically mean. Sometimes !A||B is what you logically mean, and > > implies is what you're trying to write. > > I think given that almost all other similar usage just uses DCHECK, folks > already know how to read it using DCHECK. There's a cognitive overhead of adding > all these variants. > > I don't feel super strongly about this, but IMO having fewer variants is better. +1 to both these points (I think this isn't getting a ton of usage; I don't feel super strongly about this). Another similar example: The google-internal File class grew hundreds of methods because people added many minor helper methods like "OpenOrDie()". Each of these made sense and simplified some code, but in the end there were many hundreds of methods – an end state that didn't make much sense. The lesson learned was that having some control flow with the callers isn't necessarily bad as long as it's simple and understandable. This looks somewhat similar to me.
Message was sent while issue was closed.
Another point: When a DCHECK_IMPLIES fails, it prints [8196:7024:0626/170546:826622325:FATAL:data_pack.cc(245)] Check failed: !(GetScaleForScaleFactor(handle->GetScaleFactor()) == resource_scale ) || (!handle->HasResource(resource_id)). so you need to reverse-engineer the implies logic from the gtest output.
Message was sent while issue was closed.
On 2015/06/27 00:09:31, Nico wrote: > Another point: When a DCHECK_IMPLIES fails, it prints > > [8196:7024:0626/170546:826622325:FATAL:data_pack.cc(245)] Check failed: > !(GetScaleForScaleFactor(handle->GetScaleFactor()) == resource_scale > ) || (!handle->HasResource(resource_id)). > > so you need to reverse-engineer the implies logic from the gtest output. That's true. However, removing DCHECK_IMPLIES would mean that both the author and the reader of the code would have to reverse engineer the implies logic in their head, no? Since there's a bunch of resistance to this change, I've put up a patch to remove both DCHECK_IMPLIES and CHECK_IMPLIES: https://codereview.chromium.org/1213193003/
Message was sent while issue was closed.
On 2015/06/29 18:56:38, vmpstr wrote: > On 2015/06/27 00:09:31, Nico wrote: > > Another point: When a DCHECK_IMPLIES fails, it prints > > > > [8196:7024:0626/170546:826622325:FATAL:data_pack.cc(245)] Check failed: > > !(GetScaleForScaleFactor(handle->GetScaleFactor()) == resource_scale > > ) || (!handle->HasResource(resource_id)). > > > > so you need to reverse-engineer the implies logic from the gtest output. > > That's true. However, removing DCHECK_IMPLIES would mean that both the author > and the reader of the code would have to reverse engineer the implies logic in > their head, no? > > Since there's a bunch of resistance to this change, I've put up a patch to > remove both DCHECK_IMPLIES and CHECK_IMPLIES: > https://codereview.chromium.org/1213193003/ "8K dchecks" is not a reasonable standard for comparison. There's no way that every DCHECK would ever become a DCHECK_IMPLIES. Check my math but if you just look for CHECKS/DCHECKS that are roughly of the form (!a || b), I see 316 matches: https://code.google.com/p/chromium/codesearch#search/&q=CHECK%5C(%5C!%5B%5E;%... 56 out of 316 in the codebase is actually pretty respectable for a macro that's only been around for six months. DCHECK_IMPLIES improves readability particularly for the cases where the "a" and "b" expressions are somewhat complicated and spill over to multiple lines. I prefer using it, and think a lot of the cases above would do well to migrate to it.
Message was sent while issue was closed.
On 2015/06/29 21:21:58, ncarter (gone til aug 24) wrote: > On 2015/06/29 18:56:38, vmpstr wrote: > > On 2015/06/27 00:09:31, Nico wrote: > > > Another point: When a DCHECK_IMPLIES fails, it prints > > > > > > [8196:7024:0626/170546:826622325:FATAL:data_pack.cc(245)] Check failed: > > > !(GetScaleForScaleFactor(handle->GetScaleFactor()) == resource_scale > > > ) || (!handle->HasResource(resource_id)). > > > > > > so you need to reverse-engineer the implies logic from the gtest output. > > > > That's true. However, removing DCHECK_IMPLIES would mean that both the author > > and the reader of the code would have to reverse engineer the implies logic in > > their head, no? > > > > Since there's a bunch of resistance to this change, I've put up a patch to > > remove both DCHECK_IMPLIES and CHECK_IMPLIES: > > https://codereview.chromium.org/1213193003/ > > "8K dchecks" is not a reasonable standard for comparison. There's no way that > every DCHECK would ever become a DCHECK_IMPLIES. > > Check my math but if you just look for CHECKS/DCHECKS that are roughly of the > form (!a || b), I see 316 matches: > https://code.google.com/p/chromium/codesearch#search/&q=CHECK%5C(%5C!%5B%5E;%... > > 56 out of 316 in the codebase is actually pretty respectable for a macro that's > only been around for six months. > > DCHECK_IMPLIES improves readability particularly for the cases where the "a" and > "b" expressions are somewhat complicated and spill over to multiple lines. I > prefer using it, and think a lot of the cases above would do well to migrate to > it. I'd like to ask people to keep using DCHECK_IMPLIES when it makes sense. https://codereview.chromium.org/1213193003/ has been sitting open for quite a while. Do you think we can we resolve and close it? Or approve and land it and move on? Are you any more convinced here Nico? I think ncarter's comparison at 56 out of 316 comparable uses is a pretty good argument for this being well-liked.
Message was sent while issue was closed.
On 2015/08/18 20:44:07, danakj wrote: > On 2015/06/29 21:21:58, ncarter (gone til aug 24) wrote: > > On 2015/06/29 18:56:38, vmpstr wrote: > > > On 2015/06/27 00:09:31, Nico wrote: > > > > Another point: When a DCHECK_IMPLIES fails, it prints > > > > > > > > [8196:7024:0626/170546:826622325:FATAL:data_pack.cc(245)] Check failed: > > > > !(GetScaleForScaleFactor(handle->GetScaleFactor()) == resource_scale > > > > ) || (!handle->HasResource(resource_id)). > > > > > > > > so you need to reverse-engineer the implies logic from the gtest output. > > > > > > That's true. However, removing DCHECK_IMPLIES would mean that both the > author > > > and the reader of the code would have to reverse engineer the implies logic > in > > > their head, no? > > > > > > Since there's a bunch of resistance to this change, I've put up a patch to > > > remove both DCHECK_IMPLIES and CHECK_IMPLIES: > > > https://codereview.chromium.org/1213193003/ > > > > "8K dchecks" is not a reasonable standard for comparison. There's no way that > > every DCHECK would ever become a DCHECK_IMPLIES. > > > > Check my math but if you just look for CHECKS/DCHECKS that are roughly of the > > form (!a || b), I see 316 matches: > > > https://code.google.com/p/chromium/codesearch#search/&q=CHECK%5C(%5C!%5B%5E;%... > > > > 56 out of 316 in the codebase is actually pretty respectable for a macro > that's > > only been around for six months. > > > > DCHECK_IMPLIES improves readability particularly for the cases where the "a" > and > > "b" expressions are somewhat complicated and spill over to multiple lines. I > > prefer using it, and think a lot of the cases above would do well to migrate > to > > it. > > I'd like to ask people to keep using DCHECK_IMPLIES when it makes sense. > https://codereview.chromium.org/1213193003/ has been sitting open for quite a > while. Do you think we can we resolve and close it? Or approve and land it and > move on? Are you any more convinced here Nico? > > I think ncarter's comparison at 56 out of 316 comparable uses is a pretty good > argument for this being well-liked. I was pointed to this thread when I asked in my office what the hell is DCHECK_IMPLIES? Sorry for the delayed after the fact response. If I look at something like: DCHECK(!foo || bar); I (and most likely anyone on the chrome team) immediately know what it means. When I look at DCHECK_IMPLIES I haven't a clue as to what it means. It is not readily apparent from the name what 'implies' is equivalent too. Given this, and the sparseness of DCHECK_IMPLIES I am almost guaranteed to have to look up the definition every time I see it. Worse yet, is the use of the review I'm looking at: DCHECK_IMPLIES(!params.default_file_name.empty(), params.mode == FileChooserParams::Save); Which becomes: DCHECK(!!params.default_file_name.empty(), params.mode == FileChooserParams::Save); Double negative, yum. Had the author used: DCHECK(params.default_file_name.empty() || params.mode == FileChooserParams::Save); from the get go I would easily know what the DCHECK is doing. The bar for adding functions to such a common place should be high. 'Implies' is not well defined in this context, most likely requiring engineers to search for the definition. Because of this, I think we should remove it.
Message was sent while issue was closed.
On 2015/10/21 22:40:04, sky wrote: > On 2015/08/18 20:44:07, danakj wrote: > > On 2015/06/29 21:21:58, ncarter (gone til aug 24) wrote: > > > On 2015/06/29 18:56:38, vmpstr wrote: > > > > On 2015/06/27 00:09:31, Nico wrote: > > > > > Another point: When a DCHECK_IMPLIES fails, it prints > > > > > > > > > > [8196:7024:0626/170546:826622325:FATAL:data_pack.cc(245)] Check failed: > > > > > !(GetScaleForScaleFactor(handle->GetScaleFactor()) == resource_scale > > > > > ) || (!handle->HasResource(resource_id)). > > > > > > > > > > so you need to reverse-engineer the implies logic from the gtest output. > > > > > > > > That's true. However, removing DCHECK_IMPLIES would mean that both the > > author > > > > and the reader of the code would have to reverse engineer the implies > logic > > in > > > > their head, no? > > > > > > > > Since there's a bunch of resistance to this change, I've put up a patch to > > > > remove both DCHECK_IMPLIES and CHECK_IMPLIES: > > > > https://codereview.chromium.org/1213193003/ > > > > > > "8K dchecks" is not a reasonable standard for comparison. There's no way > that > > > every DCHECK would ever become a DCHECK_IMPLIES. > > > > > > Check my math but if you just look for CHECKS/DCHECKS that are roughly of > the > > > form (!a || b), I see 316 matches: > > > > > > https://code.google.com/p/chromium/codesearch#search/&q=CHECK%5C(%5C!%5B%5E;%... > > > > > > 56 out of 316 in the codebase is actually pretty respectable for a macro > > that's > > > only been around for six months. > > > > > > DCHECK_IMPLIES improves readability particularly for the cases where the "a" > > and > > > "b" expressions are somewhat complicated and spill over to multiple lines. I > > > prefer using it, and think a lot of the cases above would do well to migrate > > to > > > it. > > > > I'd like to ask people to keep using DCHECK_IMPLIES when it makes sense. > > https://codereview.chromium.org/1213193003/ has been sitting open for quite a > > while. Do you think we can we resolve and close it? Or approve and land it and > > move on? Are you any more convinced here Nico? > > > > I think ncarter's comparison at 56 out of 316 comparable uses is a pretty good > > argument for this being well-liked. > > I was pointed to this thread when I asked in my office what the hell is > DCHECK_IMPLIES? Sorry for the delayed after the fact response. > > If I look at something like: > > DCHECK(!foo || bar); > > I (and most likely anyone on the chrome team) immediately know what it means. > > When I look at DCHECK_IMPLIES I haven't a clue as to what it means. It is not > readily apparent from the name what 'implies' is equivalent too. Given this, and > the sparseness of DCHECK_IMPLIES I am almost guaranteed to have to look up the > definition every time I see it. Worse yet, is the use of the review I'm looking > at: > > DCHECK_IMPLIES(!params.default_file_name.empty(), params.mode == > FileChooserParams::Save); > > Which becomes: > > DCHECK(!!params.default_file_name.empty(), params.mode == > FileChooserParams::Save); > > Double negative, yum. > > Had the author used: > > DCHECK(params.default_file_name.empty() || params.mode == > FileChooserParams::Save); I think they should use || here. They are saying one of these things is true. I would only suggest using it to replace !a||b, not a||b. You also wouldn't want someone to write EXPECT_FALSE(!a); You'd write EXPECT_TRUE(a), and vice versa. Or DCHECK_NE(a, !b). > from the get go I would easily know what the DCHECK is doing. DCHECK_IMPLIES(a, b) says that a implies b. I don't think this is a poorly understood logical operator? The data previously mentioned in this thread suggested that DCHECK_IMPLIES has already been almost 20% of places where it could reasonably be used (instead of DCHECK(!a || b)) within 5 months. > The bar for adding functions to such a common place should be high. 'Implies' is > not well defined in this context, most likely requiring engineers to search for > the definition. Because of this, I think we should remove it.
Message was sent while issue was closed.
On 2015/10/21 22:50:10, danakj wrote: > On 2015/10/21 22:40:04, sky wrote: > > On 2015/08/18 20:44:07, danakj wrote: > > > On 2015/06/29 21:21:58, ncarter (gone til aug 24) wrote: > > > > On 2015/06/29 18:56:38, vmpstr wrote: > > > > > On 2015/06/27 00:09:31, Nico wrote: > > > > > > Another point: When a DCHECK_IMPLIES fails, it prints > > > > > > > > > > > > [8196:7024:0626/170546:826622325:FATAL:data_pack.cc(245)] Check > failed: > > > > > > !(GetScaleForScaleFactor(handle->GetScaleFactor()) == resource_scale > > > > > > ) || (!handle->HasResource(resource_id)). > > > > > > > > > > > > so you need to reverse-engineer the implies logic from the gtest > output. > > > > > > > > > > That's true. However, removing DCHECK_IMPLIES would mean that both the > > > author > > > > > and the reader of the code would have to reverse engineer the implies > > logic > > > in > > > > > their head, no? > > > > > > > > > > Since there's a bunch of resistance to this change, I've put up a patch > to > > > > > remove both DCHECK_IMPLIES and CHECK_IMPLIES: > > > > > https://codereview.chromium.org/1213193003/ > > > > > > > > "8K dchecks" is not a reasonable standard for comparison. There's no way > > that > > > > every DCHECK would ever become a DCHECK_IMPLIES. > > > > > > > > Check my math but if you just look for CHECKS/DCHECKS that are roughly of > > the > > > > form (!a || b), I see 316 matches: > > > > > > > > > > https://code.google.com/p/chromium/codesearch#search/&q=CHECK%5C(%5C!%5B%5E;%... > > > > > > > > 56 out of 316 in the codebase is actually pretty respectable for a macro > > > that's > > > > only been around for six months. > > > > > > > > DCHECK_IMPLIES improves readability particularly for the cases where the > "a" > > > and > > > > "b" expressions are somewhat complicated and spill over to multiple lines. > I > > > > prefer using it, and think a lot of the cases above would do well to > migrate > > > to > > > > it. > > > > > > I'd like to ask people to keep using DCHECK_IMPLIES when it makes sense. > > > https://codereview.chromium.org/1213193003/ has been sitting open for quite > a > > > while. Do you think we can we resolve and close it? Or approve and land it > and > > > move on? Are you any more convinced here Nico? > > > > > > I think ncarter's comparison at 56 out of 316 comparable uses is a pretty > good > > > argument for this being well-liked. > > > > I was pointed to this thread when I asked in my office what the hell is > > DCHECK_IMPLIES? Sorry for the delayed after the fact response. > > > > If I look at something like: > > > > DCHECK(!foo || bar); > > > > I (and most likely anyone on the chrome team) immediately know what it means. > > > > When I look at DCHECK_IMPLIES I haven't a clue as to what it means. It is not > > readily apparent from the name what 'implies' is equivalent too. Given this, > and > > the sparseness of DCHECK_IMPLIES I am almost guaranteed to have to look up the > > definition every time I see it. Worse yet, is the use of the review I'm > looking > > at: > > > > DCHECK_IMPLIES(!params.default_file_name.empty(), params.mode == > > FileChooserParams::Save); > > > > Which becomes: > > > > DCHECK(!!params.default_file_name.empty(), params.mode == > > FileChooserParams::Save); > > > > Double negative, yum. > > > > Had the author used: > > > > DCHECK(params.default_file_name.empty() || params.mode == > > FileChooserParams::Save); > > I think they should use || here. They are saying one of these things is true. I > would only suggest using it to replace !a||b, not a||b. > > You also wouldn't want someone to write EXPECT_FALSE(!a); You'd write > EXPECT_TRUE(a), and vice versa. Or DCHECK_NE(a, !b). > > > from the get go I would easily know what the DCHECK is doing. > > DCHECK_IMPLIES(a, b) says that a implies b. I don't think this is a poorly > understood logical operator? If that were the case, then how come my sample of three people I mentioned this to hadn't a clue as to what it means? In this context what 'implies' means is not readily obvious. Using an || is smaller and readily understood. > The data previously mentioned in this thread > suggested that DCHECK_IMPLIES has already been almost 20% of places where it > could reasonably be used (instead of DCHECK(!a || b)) within 5 months. > > > The bar for adding functions to such a common place should be high. 'Implies' > is > > not well defined in this context, most likely requiring engineers to search > for > > the definition. Because of this, I think we should remove it. My point isn't
Message was sent while issue was closed.
On Wed, Oct 21, 2015 at 4:01 PM, <sky@chromium.org> wrote: > On 2015/10/21 22:50:10, danakj wrote: > >> On 2015/10/21 22:40:04, sky wrote: >> > On 2015/08/18 20:44:07, danakj wrote: >> > > On 2015/06/29 21:21:58, ncarter (gone til aug 24) wrote: >> > > > On 2015/06/29 18:56:38, vmpstr wrote: >> > > > > On 2015/06/27 00:09:31, Nico wrote: >> > > > > > Another point: When a DCHECK_IMPLIES fails, it prints >> > > > > > >> > > > > > [8196:7024:0626/170546:826622325:FATAL:data_pack.cc(245)] Check >> failed: >> > > > > > !(GetScaleForScaleFactor(handle->GetScaleFactor()) == >> resource_scale >> > > > > > ) || (!handle->HasResource(resource_id)). >> > > > > > >> > > > > > so you need to reverse-engineer the implies logic from the gtest >> output. >> > > > > >> > > > > That's true. However, removing DCHECK_IMPLIES would mean that >> both the >> > > author >> > > > > and the reader of the code would have to reverse engineer the >> implies >> > logic >> > > in >> > > > > their head, no? >> > > > > >> > > > > Since there's a bunch of resistance to this change, I've put up a >> > patch > >> to >> > > > > remove both DCHECK_IMPLIES and CHECK_IMPLIES: >> > > > > https://codereview.chromium.org/1213193003/ >> > > > >> > > > "8K dchecks" is not a reasonable standard for comparison. There's >> no way >> > that >> > > > every DCHECK would ever become a DCHECK_IMPLIES. >> > > > >> > > > Check my math but if you just look for CHECKS/DCHECKS that are >> roughly >> > of > >> > the >> > > > form (!a || b), I see 316 matches: >> > > > >> > > >> > >> > > > https://code.google.com/p/chromium/codesearch#search/&q=CHECK%5C(%5C!%5B%5E;%... > >> > > > >> > > > 56 out of 316 in the codebase is actually pretty respectable for a >> macro >> > > that's >> > > > only been around for six months. >> > > > >> > > > DCHECK_IMPLIES improves readability particularly for the cases >> where the >> "a" >> > > and >> > > > "b" expressions are somewhat complicated and spill over to multiple >> > lines. > >> I >> > > > prefer using it, and think a lot of the cases above would do well to >> migrate >> > > to >> > > > it. >> > > >> > > I'd like to ask people to keep using DCHECK_IMPLIES when it makes >> sense. >> > > https://codereview.chromium.org/1213193003/ has been sitting open for >> > quite > >> a >> > > while. Do you think we can we resolve and close it? Or approve and >> land it >> and >> > > move on? Are you any more convinced here Nico? >> > > >> > > I think ncarter's comparison at 56 out of 316 comparable uses is a >> pretty >> good >> > > argument for this being well-liked. >> > >> > I was pointed to this thread when I asked in my office what the hell is >> > DCHECK_IMPLIES? Sorry for the delayed after the fact response. >> > >> > If I look at something like: >> > >> > DCHECK(!foo || bar); >> > >> > I (and most likely anyone on the chrome team) immediately know what it >> > means. > >> > >> > When I look at DCHECK_IMPLIES I haven't a clue as to what it means. It >> is >> > not > >> > readily apparent from the name what 'implies' is equivalent too. Given >> this, >> and >> > the sparseness of DCHECK_IMPLIES I am almost guaranteed to have to look >> up >> > the > >> > definition every time I see it. Worse yet, is the use of the review I'm >> looking >> > at: >> > >> > DCHECK_IMPLIES(!params.default_file_name.empty(), params.mode == >> > FileChooserParams::Save); >> > >> > Which becomes: >> > >> > DCHECK(!!params.default_file_name.empty(), params.mode == >> > FileChooserParams::Save); >> > >> > Double negative, yum. >> > >> > Had the author used: >> > >> > DCHECK(params.default_file_name.empty() || params.mode == >> > FileChooserParams::Save); >> > > I think they should use || here. They are saying one of these things is >> true. >> > I > >> would only suggest using it to replace !a||b, not a||b. >> > > You also wouldn't want someone to write EXPECT_FALSE(!a); You'd write >> EXPECT_TRUE(a), and vice versa. Or DCHECK_NE(a, !b). >> > > > from the get go I would easily know what the DCHECK is doing. >> > > DCHECK_IMPLIES(a, b) says that a implies b. I don't think this is a poorly >> understood logical operator? >> > > If that were the case, then how come my sample of three people I mentioned > this > to hadn't a clue as to what it means? In this context what 'implies' means > is > not readily obvious. Using an || is smaller and readily understood. Like the argument is that the name is bad? I had no idea what DCHECK_GE was when I saw that the first time. This is why many people only know or use DCHECK() and not the variants at all. The macro names are very terse. DCHECK_EQ(a, b) means a == b. DCHECK_IMPLIES(a, b) means a => b. I dunno how it can be any more in line with the existing names which aren't amazing. There are places where writing DCHECK(!a || b) is basically not human parsable which is why this came up in the first place. You can see the removal of DCHECK_IMPLIES CL for examples, there's (I think) many places that become way more ambiguous to read. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Message was sent while issue was closed.
On Wed, Oct 21, 2015 at 4:06 PM, Dana Jansens <danakj@chromium.org> wrote: > On Wed, Oct 21, 2015 at 4:01 PM, <sky@chromium.org> wrote: > >> On 2015/10/21 22:50:10, danakj wrote: >> >>> On 2015/10/21 22:40:04, sky wrote: >>> > On 2015/08/18 20:44:07, danakj wrote: >>> > > On 2015/06/29 21:21:58, ncarter (gone til aug 24) wrote: >>> > > > On 2015/06/29 18:56:38, vmpstr wrote: >>> > > > > On 2015/06/27 00:09:31, Nico wrote: >>> > > > > > Another point: When a DCHECK_IMPLIES fails, it prints >>> > > > > > >>> > > > > > [8196:7024:0626/170546:826622325:FATAL:data_pack.cc(245)] Check >>> failed: >>> > > > > > !(GetScaleForScaleFactor(handle->GetScaleFactor()) == >>> resource_scale >>> > > > > > ) || (!handle->HasResource(resource_id)). >>> > > > > > >>> > > > > > so you need to reverse-engineer the implies logic from the >>> gtest >>> output. >>> > > > > >>> > > > > That's true. However, removing DCHECK_IMPLIES would mean that >>> both the >>> > > author >>> > > > > and the reader of the code would have to reverse engineer the >>> implies >>> > logic >>> > > in >>> > > > > their head, no? >>> > > > > >>> > > > > Since there's a bunch of resistance to this change, I've put up a >>> >> patch >> >>> to >>> > > > > remove both DCHECK_IMPLIES and CHECK_IMPLIES: >>> > > > > https://codereview.chromium.org/1213193003/ >>> > > > >>> > > > "8K dchecks" is not a reasonable standard for comparison. There's >>> no way >>> > that >>> > > > every DCHECK would ever become a DCHECK_IMPLIES. >>> > > > >>> > > > Check my math but if you just look for CHECKS/DCHECKS that are >>> roughly >>> >> of >> >>> > the >>> > > > form (!a || b), I see 316 matches: >>> > > > >>> > > >>> > >>> >> >> >> https://code.google.com/p/chromium/codesearch#search/&q=CHECK%5C(%5C!%5B%5E;%... >> >>> > > > >>> > > > 56 out of 316 in the codebase is actually pretty respectable for a >>> macro >>> > > that's >>> > > > only been around for six months. >>> > > > >>> > > > DCHECK_IMPLIES improves readability particularly for the cases >>> where the >>> "a" >>> > > and >>> > > > "b" expressions are somewhat complicated and spill over to multiple >>> >> lines. >> >>> I >>> > > > prefer using it, and think a lot of the cases above would do well >>> to >>> migrate >>> > > to >>> > > > it. >>> > > >>> > > I'd like to ask people to keep using DCHECK_IMPLIES when it makes >>> sense. >>> > > https://codereview.chromium.org/1213193003/ has been sitting open >>> for >>> >> quite >> >>> a >>> > > while. Do you think we can we resolve and close it? Or approve and >>> land it >>> and >>> > > move on? Are you any more convinced here Nico? >>> > > >>> > > I think ncarter's comparison at 56 out of 316 comparable uses is a >>> pretty >>> good >>> > > argument for this being well-liked. >>> > >>> > I was pointed to this thread when I asked in my office what the hell is >>> > DCHECK_IMPLIES? Sorry for the delayed after the fact response. >>> > >>> > If I look at something like: >>> > >>> > DCHECK(!foo || bar); >>> > >>> > I (and most likely anyone on the chrome team) immediately know what it >>> >> means. >> >>> > >>> > When I look at DCHECK_IMPLIES I haven't a clue as to what it means. It >>> is >>> >> not >> >>> > readily apparent from the name what 'implies' is equivalent too. Given >>> this, >>> and >>> > the sparseness of DCHECK_IMPLIES I am almost guaranteed to have to >>> look up >>> >> the >> >>> > definition every time I see it. Worse yet, is the use of the review I'm >>> looking >>> > at: >>> > >>> > DCHECK_IMPLIES(!params.default_file_name.empty(), params.mode == >>> > FileChooserParams::Save); >>> > >>> > Which becomes: >>> > >>> > DCHECK(!!params.default_file_name.empty(), params.mode == >>> > FileChooserParams::Save); >>> > >>> > Double negative, yum. >>> > >>> > Had the author used: >>> > >>> > DCHECK(params.default_file_name.empty() || params.mode == >>> > FileChooserParams::Save); >>> >> >> I think they should use || here. They are saying one of these things is >>> true. >>> >> I >> >>> would only suggest using it to replace !a||b, not a||b. >>> >> >> You also wouldn't want someone to write EXPECT_FALSE(!a); You'd write >>> EXPECT_TRUE(a), and vice versa. Or DCHECK_NE(a, !b). >>> >> >> > from the get go I would easily know what the DCHECK is doing. >>> >> >> DCHECK_IMPLIES(a, b) says that a implies b. I don't think this is a poorly >>> understood logical operator? >>> >> >> If that were the case, then how come my sample of three people I >> mentioned this >> to hadn't a clue as to what it means? In this context what 'implies' >> means is >> not readily obvious. Using an || is smaller and readily understood. > > > Like the argument is that the name is bad? I had no idea what DCHECK_GE > was when I saw that the first time. This is why many people only know or > use DCHECK() and not the variants at all. The macro names are very terse. > > DCHECK_EQ(a, b) means a == b. DCHECK_IMPLIES(a, b) means a => b. I dunno > how it can be any more in line with the existing names which aren't amazing. > > There are places where writing DCHECK(!a || b) is basically not human > parsable which is why this came up in the first place. You can see the > removal of DCHECK_IMPLIES CL for examples, there's (I think) many places > that become way more ambiguous to read. > If naming it after the logical operator is that bad, we could break consistency with DCHECK_EQ/GT/GE/NE..etc and call it like DCHECK_IF(a, b) instead. Maybe that would be more obvious to y'all? I just ran into some FOO_IF other macros in logging.h. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Message was sent while issue was closed.
On 2015/10/21 23:07:12, danakj wrote: > On Wed, Oct 21, 2015 at 4:01 PM, <mailto:sky@chromium.org> wrote: > > > On 2015/10/21 22:50:10, danakj wrote: > > > >> On 2015/10/21 22:40:04, sky wrote: > >> > On 2015/08/18 20:44:07, danakj wrote: > >> > > On 2015/06/29 21:21:58, ncarter (gone til aug 24) wrote: > >> > > > On 2015/06/29 18:56:38, vmpstr wrote: > >> > > > > On 2015/06/27 00:09:31, Nico wrote: > >> > > > > > Another point: When a DCHECK_IMPLIES fails, it prints > >> > > > > > > >> > > > > > [8196:7024:0626/170546:826622325:FATAL:data_pack.cc(245)] Check > >> failed: > >> > > > > > !(GetScaleForScaleFactor(handle->GetScaleFactor()) == > >> resource_scale > >> > > > > > ) || (!handle->HasResource(resource_id)). > >> > > > > > > >> > > > > > so you need to reverse-engineer the implies logic from the gtest > >> output. > >> > > > > > >> > > > > That's true. However, removing DCHECK_IMPLIES would mean that > >> both the > >> > > author > >> > > > > and the reader of the code would have to reverse engineer the > >> implies > >> > logic > >> > > in > >> > > > > their head, no? > >> > > > > > >> > > > > Since there's a bunch of resistance to this change, I've put up a > >> > > patch > > > >> to > >> > > > > remove both DCHECK_IMPLIES and CHECK_IMPLIES: > >> > > > > https://codereview.chromium.org/1213193003/ > >> > > > > >> > > > "8K dchecks" is not a reasonable standard for comparison. There's > >> no way > >> > that > >> > > > every DCHECK would ever become a DCHECK_IMPLIES. > >> > > > > >> > > > Check my math but if you just look for CHECKS/DCHECKS that are > >> roughly > >> > > of > > > >> > the > >> > > > form (!a || b), I see 316 matches: > >> > > > > >> > > > >> > > >> > > > > > > > https://code.google.com/p/chromium/codesearch#search/&q=CHECK%5C(%5C!%5B%5E;%... > > > >> > > > > >> > > > 56 out of 316 in the codebase is actually pretty respectable for a > >> macro > >> > > that's > >> > > > only been around for six months. > >> > > > > >> > > > DCHECK_IMPLIES improves readability particularly for the cases > >> where the > >> "a" > >> > > and > >> > > > "b" expressions are somewhat complicated and spill over to multiple > >> > > lines. > > > >> I > >> > > > prefer using it, and think a lot of the cases above would do well to > >> migrate > >> > > to > >> > > > it. > >> > > > >> > > I'd like to ask people to keep using DCHECK_IMPLIES when it makes > >> sense. > >> > > https://codereview.chromium.org/1213193003/ has been sitting open for > >> > > quite > > > >> a > >> > > while. Do you think we can we resolve and close it? Or approve and > >> land it > >> and > >> > > move on? Are you any more convinced here Nico? > >> > > > >> > > I think ncarter's comparison at 56 out of 316 comparable uses is a > >> pretty > >> good > >> > > argument for this being well-liked. > >> > > >> > I was pointed to this thread when I asked in my office what the hell is > >> > DCHECK_IMPLIES? Sorry for the delayed after the fact response. > >> > > >> > If I look at something like: > >> > > >> > DCHECK(!foo || bar); > >> > > >> > I (and most likely anyone on the chrome team) immediately know what it > >> > > means. > > > >> > > >> > When I look at DCHECK_IMPLIES I haven't a clue as to what it means. It > >> is > >> > > not > > > >> > readily apparent from the name what 'implies' is equivalent too. Given > >> this, > >> and > >> > the sparseness of DCHECK_IMPLIES I am almost guaranteed to have to look > >> up > >> > > the > > > >> > definition every time I see it. Worse yet, is the use of the review I'm > >> looking > >> > at: > >> > > >> > DCHECK_IMPLIES(!params.default_file_name.empty(), params.mode == > >> > FileChooserParams::Save); > >> > > >> > Which becomes: > >> > > >> > DCHECK(!!params.default_file_name.empty(), params.mode == > >> > FileChooserParams::Save); > >> > > >> > Double negative, yum. > >> > > >> > Had the author used: > >> > > >> > DCHECK(params.default_file_name.empty() || params.mode == > >> > FileChooserParams::Save); > >> > > > > I think they should use || here. They are saying one of these things is > >> true. > >> > > I > > > >> would only suggest using it to replace !a||b, not a||b. > >> > > > > You also wouldn't want someone to write EXPECT_FALSE(!a); You'd write > >> EXPECT_TRUE(a), and vice versa. Or DCHECK_NE(a, !b). > >> > > > > > from the get go I would easily know what the DCHECK is doing. > >> > > > > DCHECK_IMPLIES(a, b) says that a implies b. I don't think this is a poorly > >> understood logical operator? > >> > > > > If that were the case, then how come my sample of three people I mentioned > > this > > to hadn't a clue as to what it means? In this context what 'implies' means > > is > > not readily obvious. Using an || is smaller and readily understood. > > > Like the argument is that the name is bad? I had no idea what DCHECK_GE was > when I saw that the first time. This is why many people only know or use > DCHECK() and not the variants at all. The macro names are very terse. I agree with terseness, but once you see that GE is short for greater than or equal it's easy to understand from that point on. 'Implies' is still not obvious to me and I know I'm going to have to look it up every time. I don't think I'm alone here. > DCHECK_EQ(a, b) means a == b. DCHECK_IMPLIES(a, b) means a => b. I dunno > how it can be any more in line with the existing names which aren't amazing. > > There are places where writing DCHECK(!a || b) is basically not human > parsable which is why this came up in the first place. You can see the > removal of DCHECK_IMPLIES CL for examples, there's (I think) many places > that become way more ambiguous to read. > > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org.
Message was sent while issue was closed.
Description was changed from ========== base: Add CHECK_IMPLIES and DCHECK_IMPLIES. This patch adds convenience defines for implies relationship. a implies b is equivalent to !a || b. This allows things like the following: CHECK_IMPLIES(ptr, ptr->is_good); instead of the more cumbersome: CHECK(!ptr || ptr->is_good). Committed: https://crrev.com/0456d13053908dc3929035e4abf8799c6e9627a5 Cr-Commit-Position: refs/heads/master@{#297941} ========== to ========== base: Add CHECK_IMPLIES and DCHECK_IMPLIES. This patch adds convenience defines for implies relationship. a implies b is equivalent to !a || b. This allows things like the following: CHECK_IMPLIES(ptr, ptr->is_good); instead of the more cumbersome: CHECK(!ptr || ptr->is_good). Committed: https://crrev.com/0456d13053908dc3929035e4abf8799c6e9627a5 Cr-Commit-Position: refs/heads/master@{#297941} ==========
Message was sent while issue was closed.
On 2015/10/21 23:24:28, sky wrote: > > > DCHECK_IMPLIES(a, b) says that a implies b. I don't think this is a poorly > > >> understood logical operator? > > >> > > > > > > If that were the case, then how come my sample of three people I mentioned > > > this > > > to hadn't a clue as to what it means? In this context what 'implies' means > > > is > > > not readily obvious. Using an || is smaller and readily understood. > > > > > > Like the argument is that the name is bad? I had no idea what DCHECK_GE was > > when I saw that the first time. This is why many people only know or use > > DCHECK() and not the variants at all. The macro names are very terse. > > I agree with terseness, but once you see that GE is short for greater than or > equal it's easy to understand from that point on. 'Implies' is still not obvious > to me and I know I'm going to have to look it up every time. I don't think I'm > alone here. > > > DCHECK_EQ(a, b) means a == b. DCHECK_IMPLIES(a, b) means a => b. I dunno > > how it can be any more in line with the existing names which aren't amazing. > > > > There are places where writing DCHECK(!a || b) is basically not human > > parsable which is why this came up in the first place. You can see the > > removal of DCHECK_IMPLIES CL for examples, there's (I think) many places > > that become way more ambiguous to read. Nick who is good at regexs gave me this: https://code.google.com/p/chromium/codesearch#search/&q=if%5C%20%5C(%5B%5E%7B... These are places where people wrote if (A) then DCHECK(B). But now you have code that isn't used for anything but dchecking outside of a dcheck. Before, you're correctly rewrite those as DCHECK(!A || B). But these weren't written that way in some cases I assume because they become unparseable. Here's a single concrete example: https://code.google.com/p/chromium/codesearch#chromium/src/ui/wm/core/focus_c... if (active_window_ && focusable) DCHECK(active_window_->Contains(focusable)); This isn't quite right, we should put it all in the DCHECK: DCHECK(!(active_window_ && focusable) || active_window_->Contains(focusable)); Parsing this in my head is.. hard. What does that mean? (Without looking at the if statement above.) It requires a lot of cognitive load to get the meaning here. DCHECK_IMPLIES(active_window_ && focusable, active_window_->Contains(focusable)); This one says exactly what we started out with. If active_window_ and focusable, then the Contains function is true. I feel like this is very cut-and-dry, like there's no way I'd want someone writing that kind of DCHECK without implies. I'd maybe make them wrap the whole thing in #if DCHECK_IS_ON(), which is just a lot of noise in comparison. I see these type of complicated DCHECKs a lot, and the way you process it in your head is to turn it into if (A) then DCHECK(B). Which is exactly what DCHECK_IMPLIES does for you without making you negate things. You didn't reply to my comment about DCHECK_IF. There's no implies operator in c++, but there is an if keyword. IDK if that would help. I'd accept renaming it but I think it would be a degradation of our code to remove it. And other devs were very unhappy when they heard it might go away.
Message was sent while issue was closed.
By now, three people (sky, jam, I) have independently and pretty consistently said that they thought this was confusing and not base-worthy. (See also https://codereview.chromium.org/1213193003/ for some more comments in that vein.) I wasn't really sure if I was correct, but I think the consistent feedback paints a fairly clear picture by now. Let's rip this out. (Since several folks think it's confusing, the macro being kind of widely used isn't necessarily a good thing imho.)
Message was sent while issue was closed.
On 2015/10/22 02:01:16, Nico wrote: > By now, three people (sky, jam, I) have independently and pretty consistently > said that they thought this was confusing and not base-worthy. Though a greater number of people have found it useful and used it and/or complained when we talked of removing it? But can you review the CL to remove it then and take a look at the callsites there? > (See also https://codereview.chromium.org/1213193003/ for some more comments in > that vein.) > > I wasn't really sure if I was correct, but I think the consistent feedback > paints a fairly clear picture by now. Let's rip this out. > > (Since several folks think it's confusing, the macro being kind of widely used > isn't necessarily a good thing imho.) |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
