NOTREACHED() usage

199 views
Skip to first unread message

Thiago Farina

unread,
May 14, 2017, 12:22:19 PM5/14/17
to chromi...@chromium.org
Hello,

If a function [1] can fail, even if it is unlikely, is
NOTREACHED() usage correct?

One example is:

if (!getcwd(system_buffer, sizeof(system_buffer))) {
NOTREACHED();
return false;
}

Do have guidelines for logging code path errors?

[1] - http://pubs.opengroup.org/onlinepubs/009695399/functions/getcwd.html

Charles Harrison

unread,
May 14, 2017, 12:31:18 PM5/14/17
to tfa...@chromium.org, Chromium-dev
Please see the style guide for this. You should not put NOTREACHED() in a condition that can sometimes occur.

There's additional rules on adding logging but the general rule is to avoid it unless really necessary.


--
--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
    http://groups.google.com/a/chromium.org/group/chromium-dev
---
You received this message because you are subscribed to the Google Groups "Chromium-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to chromium-dev+unsubscribe@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/20170514162029.GA17552%40diavel.


Thiago Farina

unread,
May 14, 2017, 3:31:19 PM5/14/17
to Charles Harrison, chromi...@chromium.org
On Sun, May 14, 2017 at 12:29:56PM -0400, Charles Harrison wrote:
> Please see the style guide
> <https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md#CHECK_DCHECK_and-NOTREACHED>
> for this. You should not put NOTREACHED() in a condition that can sometimes
> occur.
>
Is OK to use PLOG(ERROR) in these cases?

Charles Harrison

unread,
May 14, 2017, 9:09:11 PM5/14/17
to Thiago Farina, Chromium-dev
I think the link I mentioned before about when to add logging messages is the only standard Chromium uses. I may be missing something though.

You *can* add PLOGs if you really need to, but there's encouragement to avoid logging if possible or use DLOG (DPLOG in your case) instead to avoid bloating the release binary.

Primiano Tucci

unread,
May 15, 2017, 12:45:34 AM5/15/17
to cshar...@chromium.org, tfa...@chromium.org, Chromium-dev
On Sun, May 14, 2017 at 9:30 AM Charles Harrison <cshar...@chromium.org> wrote:
Please see the style guide for this. You should not put NOTREACHED() in a condition that can sometimes occur.

Hmm, there is a use case which, I think, doesn't seem covered by the style guide and I'd like to have some though on: a public method where the intent is:
- NOTREACHED(): signal that the client code calling this has a bug and should be fixed.
- return: the actual API can deal with that failure without security/stability implications.

Essentially for cases where I want a bug to be filed if the condition happens but are not critical enough to justify impacting the user experience with a hard crash. Specific example for me: [1]

In this regard the style guide says:
-  A DCHECK() means “this condition must always be true”, not “this condition is normally true, but perhaps not in exceptional cases.”. Which holds in this case, i am signaling that the client code has a bug and should never hit that code path. 
- Use CHECK() if the consequence of a failed assertion would be a security vulnerability: there is no security side-effect here, no-oping the api solves the issue.

the NOTREACHED is to prevent that client to expensive operations to produce data that we'll drop on the floor in MemoryDumpLevelOfDetail::BACKGROUND dumps. It's not a drama if it happens (I don't feel I should cause a crash) but I'd like to know and fix the client code when it happens (ironically just happened today crbug.com/722052).
 
Thoughts?


There's additional rules on adding logging but the general rule is to avoid it unless really necessary.

On Sun, May 14, 2017 at 12:20 PM, Thiago Farina <tfa...@chromium.org> wrote:
Hello,

If a function [1] can fail, even if it is unlikely, is
NOTREACHED() usage correct?

One example is:

if (!getcwd(system_buffer, sizeof(system_buffer))) {
  NOTREACHED();
  return false;
}

Do have guidelines for logging code path errors?

[1] - http://pubs.opengroup.org/onlinepubs/009695399/functions/getcwd.html

--
--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
    http://groups.google.com/a/chromium.org/group/chromium-dev
---
You received this message because you are subscribed to the Google Groups "Chromium-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to chromium-dev...@chromium.org.
--
--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev
---
You received this message because you are subscribed to the Google Groups "Chromium-dev" group.

Charles Harrison

unread,
May 15, 2017, 8:24:26 AM5/15/17
to Primiano Tucci, Thiago Farina, Chromium-dev
IMO that usage is fine, as it is encoding pre-conditions that the API supports. As long as you've documented this I think the code is following the style guide.

Another way of thinking about it is that DCHECK failures in Chromium are treated as P1 errors that need to be addressed immediately since they represent broken contracts / behavior. A DCHECK failure on network/disk failure could never be fixed.

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

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/20170514162029.GA17552%40diavel.

--
--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev
---
You received this message because you are subscribed to the Google Groups "Chromium-dev" group.

Peter Kasting

unread,
May 15, 2017, 1:19:04 PM5/15/17
to Primiano Tucci, Charles Harrison, tfarina, Chromium-dev
On Sun, May 14, 2017 at 9:44 PM, Primiano Tucci <prim...@chromium.org> wrote:
On Sun, May 14, 2017 at 9:30 AM Charles Harrison <cshar...@chromium.org> wrote:
Please see the style guide for this. You should not put NOTREACHED() in a condition that can sometimes occur.

Hmm, there is a use case which, I think, doesn't seem covered by the style guide and I'd like to have some though on: a public method where the intent is:
- NOTREACHED(): signal that the client code calling this has a bug and should be fixed.
- return: the actual API can deal with that failure without security/stability implications.

Essentially for cases where I want a bug to be filed if the condition happens but are not critical enough to justify impacting the user experience with a hard crash. Specific example for me: [1]

In this regard the style guide says:
-  A DCHECK() means “this condition must always be true”, not “this condition is normally true, but perhaps not in exceptional cases.”. Which holds in this case, i am signaling that the client code has a bug and should never hit that code path. 
- Use CHECK() if the consequence of a failed assertion would be a security vulnerability: there is no security side-effect here, no-oping the api solves the issue.

the NOTREACHED is to prevent that client to expensive operations to produce data that we'll drop on the floor in MemoryDumpLevelOfDetail::BACKGROUND dumps. It's not a drama if it happens (I don't feel I should cause a crash) but I'd like to know and fix the client code when it happens (ironically just happened today crbug.com/722052).
 
Thoughts?

I think this code should be written as:

DCHECK_NE(MemoryDumpLevelOfDetail::BACKGROUND,
          process_memory_dump_->dump_args().level_of_detail);
... continue execution, do not return

This is equivalent to the existing code in terms of when it crashes, but it does not write "handler" code for the case where the assertion fails.  Such handler code is banned by our team style guide.

AIUI, lacking that handler will not lead to the rest of the code crashing or something, just to doing more work than is necessary; but since the DCHECK asserts that this literally never happens, that's fine.

PK

Primiano Tucci

unread,
May 16, 2017, 12:05:09 AM5/16/17
to Peter Kasting, Charles Harrison, tfarina, Chromium-dev
On Mon, May 15, 2017 at 10:18 AM Peter Kasting <pkas...@chromium.org> wrote:
On Sun, May 14, 2017 at 9:44 PM, Primiano Tucci <prim...@chromium.org> wrote:
On Sun, May 14, 2017 at 9:30 AM Charles Harrison <cshar...@chromium.org> wrote:
Please see the style guide for this. You should not put NOTREACHED() in a condition that can sometimes occur.

Hmm, there is a use case which, I think, doesn't seem covered by the style guide and I'd like to have some though on: a public method where the intent is:
- NOTREACHED(): signal that the client code calling this has a bug and should be fixed.
- return: the actual API can deal with that failure without security/stability implications.

Essentially for cases where I want a bug to be filed if the condition happens but are not critical enough to justify impacting the user experience with a hard crash. Specific example for me: [1]

In this regard the style guide says:
-  A DCHECK() means “this condition must always be true”, not “this condition is normally true, but perhaps not in exceptional cases.”. Which holds in this case, i am signaling that the client code has a bug and should never hit that code path. 
- Use CHECK() if the consequence of a failed assertion would be a security vulnerability: there is no security side-effect here, no-oping the api solves the issue.

the NOTREACHED is to prevent that client to expensive operations to produce data that we'll drop on the floor in MemoryDumpLevelOfDetail::BACKGROUND dumps. It's not a drama if it happens (I don't feel I should cause a crash) but I'd like to know and fix the client code when it happens (ironically just happened today crbug.com/722052).
 
Thoughts?

I think this code should be written as:

DCHECK_NE(MemoryDumpLevelOfDetail::BACKGROUND,
          process_memory_dump_->dump_args().level_of_detail);
... continue execution, do not return

Continuing the execution will have some undesirable side effects like potentially leaking PII in traces that are not supposed to contain any PII.
From my viewpoint the only feasible options are CHECK() (hard crash) or DCHECK()/NOTREACHED() + return 
 
This is equivalent to the existing code in terms of when it crashes, but it does not write "handler" code for the case where the assertion fails.  Such handler code is banned by our team style guide.

This is the question I am having. I can't figure out if that is banned in general or only when trying to catch cases that shouldn't happen % exceptional cases (I/O errors or such).
Mine is not a matter of "% exceptional cases". Shouldn't happen, period. Just, if it happens, I have a way to deal with that that doesn't make the browser crash and disappear from the user.
 
AIUI, lacking that handler will not lead to the rest of the code crashing or something, just to doing more work than is necessary;

Oh, I definitely omitted some context sorry. There is an extra problem:
there would be PII implications without that return. Essentially for BAKGROUND level I have to consider anything which is not a memory size as something that could potentially leak URLs or similar.
With that return, the client code will do some more work than necessary (hence the NOTREACHED() to warn them) which is not a huge problem, as long as I drop the string on the floor (or crash).
If I remove the notreached nobody will never realize that the client code is doing useless work and nobody is ever going to fix it. 

Peter Kasting

unread,
May 16, 2017, 1:49:56 AM5/16/17
to Primiano Tucci, Charles Harrison, tfarina, Chromium-dev
On Mon, May 15, 2017 at 9:04 PM, Primiano Tucci <prim...@chromium.org> wrote:
On Mon, May 15, 2017 at 10:18 AM Peter Kasting <pkas...@chromium.org> wrote:
On Sun, May 14, 2017 at 9:44 PM, Primiano Tucci <prim...@chromium.org> wrote:
On Sun, May 14, 2017 at 9:30 AM Charles Harrison <cshar...@chromium.org> wrote:
Please see the style guide for this. You should not put NOTREACHED() in a condition that can sometimes occur.

Hmm, there is a use case which, I think, doesn't seem covered by the style guide and I'd like to have some though on: a public method where the intent is:
- NOTREACHED(): signal that the client code calling this has a bug and should be fixed.
- return: the actual API can deal with that failure without security/stability implications.

Essentially for cases where I want a bug to be filed if the condition happens but are not critical enough to justify impacting the user experience with a hard crash. Specific example for me: [1]

In this regard the style guide says:
-  A DCHECK() means “this condition must always be true”, not “this condition is normally true, but perhaps not in exceptional cases.”. Which holds in this case, i am signaling that the client code has a bug and should never hit that code path. 
- Use CHECK() if the consequence of a failed assertion would be a security vulnerability: there is no security side-effect here, no-oping the api solves the issue.

the NOTREACHED is to prevent that client to expensive operations to produce data that we'll drop on the floor in MemoryDumpLevelOfDetail::BACKGROUND dumps. It's not a drama if it happens (I don't feel I should cause a crash) but I'd like to know and fix the client code when it happens (ironically just happened today crbug.com/722052).
 
Thoughts?

I think this code should be written as:

DCHECK_NE(MemoryDumpLevelOfDetail::BACKGROUND,
          process_memory_dump_->dump_args().level_of_detail);
... continue execution, do not return

Continuing the execution will have some undesirable side effects like potentially leaking PII in traces that are not supposed to contain any PII.

...if the DCHECK is violated.  Which it isn't, because that's the point of the DCHECK.

If you think it could conceivably be violated, you cannot use any of DCHECK, CHECK, or NOTREACHED; you must use a conditional.  The assertion can be violated or it can't, there is no "it can't, but handle when it can".
 
From my viewpoint the only feasible options are CHECK() (hard crash) or DCHECK()/NOTREACHED() + return 
 
This is equivalent to the existing code in terms of when it crashes, but it does not write "handler" code for the case where the assertion fails.  Such handler code is banned by our team style guide.

This is the question I am having. I can't figure out if that is banned in general or only when trying to catch cases that shouldn't happen % exceptional cases (I/O errors or such).

It is always banned.

(Note that I/O errors are an explicit case that should be considered as "can happen", and one should never have a DCHECK that could be violated due to an I/O error.)
 
Mine is not a matter of "% exceptional cases". Shouldn't happen, period. Just, if it happens,

The style guide bans arguments that put those last three words after the previous three :)
 
AIUI, lacking that handler will not lead to the rest of the code crashing or something, just to doing more work than is necessary;

Oh, I definitely omitted some context sorry. There is an extra problem:
there would be PII implications without that return. Essentially for BAKGROUND level I have to consider anything which is not a memory size as something that could potentially leak URLs or similar.

This is so obscure it's not really different (IMO) than "well, any memory corruption bug could potentially leak PII to something".  I would consider this Don't Care.  If you think I am wrong, I think the counterargument you should make is that this is so likely to leak PII that it deserves to be a CHECK.  I don't think you should make that argument though.  (And note that "leaks PII" is not a condition listed as rationale for upgrading to CHECK.)
 
With that return, the client code will do some more work than necessary (hence the NOTREACHED() to warn them)

NOTREACHED and DCHECK are not meant to serve the purpose of warning anyone anything or catching errors.  They are not intended to alert developers to problems; they're intended as documentation to readers that something Shall Not Happen.  If you want a mechanism to find and catch potential errors, you need something outside this family of functions, or a time-limited CHECK.
 
If I remove the notreached nobody will never realize that the client code is doing useless work and nobody is ever going to fix it.

The intent is that developers assume no one will ever catch that with a NOTREACHED either, since, again, it's not preventative.  We do not intend that sufficient usage of the browser happens with DCHECKs enabled to somehow "exercise" them all.

PK

Alexei Svitkine

unread,
May 16, 2017, 9:56:48 AM5/16/17
to Peter Kasting, Primiano Tucci, Charles Harrison, tfarina, Chromium-dev
(from correct email this time)

On May 16, 2017 9:54 AM, "Alexei Svitkine" <asvi...@google.com> wrote:
I think if the concern is indeed "leak of PII" then it should be a CHECK.

Don't think of the problem of crashing the user - instead consider that the CHECK guarantees that you find about any issues in the wild. When you find them and fix them, then the concern about crashing the user becomes moot - because now it won't trigger.

Again above applies because the consequence otherwise as you've said is leaking PII which is very severe.

--
--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev
---
You received this message because you are subscribed to the Google Groups "Chromium-dev" group.

Primiano Tucci

unread,
May 16, 2017, 11:54:15 AM5/16/17
to Peter Kasting, Charles Harrison, tfarina, Chromium-dev
On Mon, May 15, 2017 at 10:49 PM Peter Kasting <pkas...@chromium.org> wrote:
On Mon, May 15, 2017 at 9:04 PM, Primiano Tucci <prim...@chromium.org> wrote:
On Mon, May 15, 2017 at 10:18 AM Peter Kasting <pkas...@chromium.org> wrote:
On Sun, May 14, 2017 at 9:44 PM, Primiano Tucci <prim...@chromium.org> wrote:
On Sun, May 14, 2017 at 9:30 AM Charles Harrison <cshar...@chromium.org> wrote:
Please see the style guide for this. You should not put NOTREACHED() in a condition that can sometimes occur.

Hmm, there is a use case which, I think, doesn't seem covered by the style guide and I'd like to have some though on: a public method where the intent is:
- NOTREACHED(): signal that the client code calling this has a bug and should be fixed.
- return: the actual API can deal with that failure without security/stability implications.

Essentially for cases where I want a bug to be filed if the condition happens but are not critical enough to justify impacting the user experience with a hard crash. Specific example for me: [1]

In this regard the style guide says:
-  A DCHECK() means “this condition must always be true”, not “this condition is normally true, but perhaps not in exceptional cases.”. Which holds in this case, i am signaling that the client code has a bug and should never hit that code path. 
- Use CHECK() if the consequence of a failed assertion would be a security vulnerability: there is no security side-effect here, no-oping the api solves the issue.

the NOTREACHED is to prevent that client to expensive operations to produce data that we'll drop on the floor in MemoryDumpLevelOfDetail::BACKGROUND dumps. It's not a drama if it happens (I don't feel I should cause a crash) but I'd like to know and fix the client code when it happens (ironically just happened today crbug.com/722052).
 
Thoughts?

I think this code should be written as:

DCHECK_NE(MemoryDumpLevelOfDetail::BACKGROUND,
          process_memory_dump_->dump_args().level_of_detail);
... continue execution, do not return

Continuing the execution will have some undesirable side effects like potentially leaking PII in traces that are not supposed to contain any PII.

...if the DCHECK is violated.  Which it isn't, because that's the point of the DCHECK.

Whether the DCHECK is violated is not up to me but to clients.
 

If you think it could conceivably be violated, you cannot use any of DCHECK, CHECK, or NOTREACHED; you must use a conditional.
How am I going know then that clients are burning cpu  and memory to do useless stuff that my conditional is going to drop on the dloor?
 
  The assertion can be violated or it can't, there is no "it can't, but handle when it can".

The assertion can be violated by other parts of the codebase I don't control (and when it happens is a bug)
 
 
From my viewpoint the only feasible options are CHECK() (hard crash) or DCHECK()/NOTREACHED() + return 
 
This is equivalent to the existing code in terms of when it crashes, but it does not write "handler" code for the case where the assertion fails.  Such handler code is banned by our team style guide.

This is the question I am having. I can't figure out if that is banned in general or only when trying to catch cases that shouldn't happen % exceptional cases (I/O errors or such).

It is always banned.

(Note that I/O errors are an explicit case that should be considered as "can happen", and one should never have a DCHECK that could be violated due to an I/O error.)

What I am saying is that I buy that we shouldn't try to handle (notreached+return) things like I/O error because are outside of the control of our codebase. I am not convinced that the same rule should apply for things that we control.
 
 
Mine is not a matter of "% exceptional cases". Shouldn't happen, period. Just, if it happens,

The style guide bans arguments that put those last three words after the previous three :)

Well, if there is agreement about this, I think we should have a presubmit. One thing that I noticed only now is that there are 440 instances of this pattern in the codebase.
 
 
AIUI, lacking that handler will not lead to the rest of the code crashing or something, just to doing more work than is necessary;

Oh, I definitely omitted some context sorry. There is an extra problem:
there would be PII implications without that return. Essentially for BAKGROUND level I have to consider anything which is not a memory size as something that could potentially leak URLs or similar.

This is so obscure it's not really different (IMO) than "well, any memory corruption bug could potentially leak PII to something".
Disagree. That is just a matter of somebody refactoring / adding new code in the clients and forgetting to deal properly with BACKGROUND mode. It happened for real yesterday here on crbug.com/722052
 
  I would consider this Don't Care. 
I care about not leaking PII by accident.
 
If you think I am wrong, I think the counterargument you should make is that this is so likely to leak PII that it deserves to be a CHECK.
Yup it seems this is the only way then.
 
  I don't think you should make that argument though.  (And note that "leaks PII" is not a condition listed as rationale for upgrading to CHECK.)
 
With that return, the client code will do some more work than necessary (hence the NOTREACHED() to warn them)

NOTREACHED and DCHECK are not meant to serve the purpose of warning anyone anything or catching errors.  They are not intended to alert developers to problems
they're intended as documentation to readers that something Shall Not Happen. 
I am not sure I agree with this. If that's the case just use // comments. The all point of DCHECKs for me is getting bugs and fixing code that doesn't behave like it should.
 
If you want a mechanism to find and catch potential errors, you need something outside this family of functions,
such as?
 
or a time-limited CHECK.
What happens if people regress this again after I remove the CHECK? I
 
 
If I remove the notreached nobody will never realize that the client code is doing useless work and nobody is ever going to fix it.

The intent is that developers assume no one will ever catch that with a NOTREACHED either, since, again, it's not preventative. 
This doesn't seem to be the case for most DHCEKS I hit.
Random example:
(warn develoeprs that pass 0 as |count|)

Thiago Farina

unread,
May 16, 2017, 12:45:26 PM5/16/17
to Primiano Tucci, Peter Kasting, Charles Harrison, Chromium-dev
On Tue, May 16, 2017 at 12:53 PM, Primiano Tucci <prim...@chromium.org> wrote:


On Mon, May 15, 2017 at 10:49 PM Peter Kasting <pkas...@chromium.org> wrote:
On Mon, May 15, 2017 at 9:04 PM, Primiano Tucci <prim...@chromium.org> wrote:
Mine is not a matter of "% exceptional cases". Shouldn't happen, period. Just, if it happens,

The style guide bans arguments that put those last three words after the previous three :)

Well, if there is agreement about this, I think we should have a presubmit. One thing that I noticed only now is that there are 440 instances of this pattern in the codebase.
 
There are much more than this when you look for "return *;" [1] as well.  :)


--
Thiago Farina

Peter Kasting

unread,
May 16, 2017, 1:35:00 PM5/16/17
to Primiano Tucci, Charles Harrison, tfarina, Chromium-dev
On Tue, May 16, 2017 at 8:53 AM, Primiano Tucci <prim...@chromium.org> wrote:
On Mon, May 15, 2017 at 10:49 PM Peter Kasting <pkas...@chromium.org> wrote:
...if the DCHECK is violated.  Which it isn't, because that's the point of the DCHECK.

Whether the DCHECK is violated is not up to me but to clients.

Is this condition well-documented?  Is it part of the contract a responsible client would understand?  Does it honestly not make sense for a caller to do this?  Does existing caller code (that people would look to as an example) do the right thing and make it clear why?  If the answers to these questions are yes, then at some point you have to trust your teammates.  If one or more is no, help by making your API as easy to use correctly and as hard to use incorrectly as possible.

Basically, when a bug is introduced, what's the postmortem rationale for why the bug occurred?  How can we make the system better such that people are unlikely to use it wrong, rather than just hoping, and trying to catch errors when they do occur?

If you think it could conceivably be violated, you cannot use any of DCHECK, CHECK, or NOTREACHED; you must use a conditional.
How am I going know then that clients are burning cpu  and memory to do useless stuff that my conditional is going to drop on the dloor?

This goes back to the above paragraphs.

Your responsibility is to build APIs that make sense and callers are likely to understand and use well, but not to guarantee that they are, in fact, using them well.  That part is their responsibility.  Often when a bug occurs, both sides could have done better.

Mine is not a matter of "% exceptional cases". Shouldn't happen, period. Just, if it happens,

The style guide bans arguments that put those last three words after the previous three :)

Well, if there is agreement about this, I think we should have a presubmit. One thing that I noticed only now is that there are 440 instances of this pattern in the codebase.

Adding a presubmit would be OK with me.  Note that 440 instances across 10 million lines is extremely low (and 60 of those go away when you exclude third_party).  If you expand to Thiago's suggestion (but still exclude third_party), it goes up to 2105, which is more concerning, but still very uncommon.  And there are cases where it actually makes sense to write code like this (especially in the default cases of a switches) because such a return is needed. But in general, yes, I would be keen to not see this pattern copied.

If you think I am wrong, I think the counterargument you should make is that this is so likely to leak PII that it deserves to be a CHECK.
Yup it seems this is the only way then.

Go for it.

If you want a mechanism to find and catch potential errors, you need something outside this family of functions,
such as?

Returning error codes to callers along with WARN_UNUSED_RESULT; logging; etc.

or a time-limited CHECK.
What happens if people regress this again after I remove the CHECK?

If people are commonly regressing something code asserts should never happen, I return to the question of whether the API and its contracts are clearly stated (and correctly chosen to begin with).

If I remove the notreached nobody will never realize that the client code is doing useless work and nobody is ever going to fix it.

The intent is that developers assume no one will ever catch that with a NOTREACHED either, since, again, it's not preventative. 
This doesn't seem to be the case for most DHCEKS I hit.
Random example:
(warn develoeprs that pass 0 as |count|)

Are you sure the person who wrote that was really intending to "warn developers" of something?  While I agree that this shouldn't have been written with NOTREACHED, it's not clear what the intent of the author was.  (Which is part of why this practice is banned.)

PK

Anthony Berent

unread,
May 16, 2017, 2:06:03 PM5/16/17
to pkas...@chromium.org, Primiano Tucci, Charles Harrison, tfarina, Chromium-dev
The check here makes a lot of sense to me. Calling addCount with a count of 0 is breaking the API contract, but however well this is documented doing so is always going to be an easy mistake to make. As such it is sensible, and helpful to users of the API, to have a DCHECK for this condition.
PK

--
--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev
---
You received this message because you are subscribed to the Google Groups "Chromium-dev" group.

Antoine Labour

unread,
May 16, 2017, 2:12:01 PM5/16/17
to Anthony Berent, Peter Kasting, Primiano Tucci, Charles Harrison, tfarina, Chromium-dev
ISTM the API contract was changed to accept 0 (making the API more useful, since "it's always going to be an easy mistake to make"), and the NOTREACHED shouldn't be there. The semantics are well defined for a count of 0.
The status-quo seems the worst of both worlds, because the callers need to explicitly test for 0, and the implementation also ends up explicitly testing for 0.

Antoine
 
PK

--
--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev
---
You received this message because you are subscribed to the Google Groups "Chromium-dev" group.
 To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAAHOzFAXXP31dWYk3yGV7fQW9%2BTx-5BJ8%2ByCpWRmnd0m3WLh8g%40mail.gmail.com.

--
--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev
---
You received this message because you are subscribed to the Google Groups "Chromium-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to chromium-dev+unsubscribe@chromium.org.
 To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CABdZ6yDMFZR83pUkovZdOZQfLq%3DWX%3D8uHW5q-NfZn5WMVrQqBQ%40mail.gmail.com.

Peter Kasting

unread,
May 16, 2017, 2:16:58 PM5/16/17
to Antoine Labour, Anthony Berent, Primiano Tucci, Charles Harrison, tfarina, Chromium-dev
On Tue, May 16, 2017 at 11:10 AM, Antoine Labour <pi...@google.com> wrote:
ISTM the API contract was changed to accept 0 (making the API more useful, since "it's always going to be an easy mistake to make"), and the NOTREACHED shouldn't be there. The semantics are well defined for a count of 0.
The status-quo seems the worst of both worlds, because the callers need to explicitly test for 0, and the implementation also ends up explicitly testing for 0.

Right.  This is a case where I think it makes more sense to just silently allow the caller to pass 0.  There's no particular reason it needs to be excluded.

PK 
Reply all
Reply to author
Forward
0 new messages