Deferencing a Pointer Doesn't Guarantee a Crash on a nullptr. A DCHECK is Still Necessary.

[Robert Liao]

tl;dr - Code that immediately dereferences an incoming pointer may not necessarily crash on a nullptr. You still need to DCHECK that pointer to guarantee a crash depending on how you dereference the pointer.

This investigation stemmed from a code review where we were discussing whether or not a DCHECK would be redundant for code that immediately dereferences the pointer.

As we all know, dereferencing a nullptr in the land of C++ results in undefined behavior. Sometimes, we take for granted that this generally results in a crash, but that is not always the case!

Considering the following code:

class SomeObject {

 public:

  SomeObject(const SomeObject& other) {}

  void DoNothing() {}

};

struct A {

  SomeObject some_object;

  SomeObject* pointer;

};

void DoesThisCrash1(A* a) {

  SomeObject* some_object = &a->some_object;

  a->some_object.DoNothing();

  (*a).some_object.DoNothing();

}

void DoesThisCrash2(A* a) {

  SomeObject some_object(a->some_object);

}

void DoesThisCrash3(A* a) {

  SomeObject* some_object = a->pointer;

  some_object->DoNothing();

  a->pointer->DoNothing();

}

int main(int argc, char* argv[]) {

  DoesThisCrash1(nullptr);

  DoesThisCrash2(nullptr);

  DoesThisCrash3(nullptr);

}

So, where does this program crash for, say, the MSVC Compiler?

For an inside look, we'll need to look at the resulting assembly. In this case, it's Intel style x86.

void DoesThisCrash1(A* a) {

  SomeObject* some_object = &a->some_object;

      mov     eax,dword ptr [ebp+8]    eax = 0

      mov     dword ptr [ebp-8],eax

  a->some_object.DoNothing();

      mov     ecx,dword ptr [ebp+8]    ecx = this = a = 0

      call    SomeObject::DoNothing

  (*a).some_object.DoNothing();

      mov     ecx,dword ptr [ebp+8]    ecx = this = a = 0

      call    SomeObject::DoNothing

}

As you can see from the assembly, we actually never dereference the nullptr, even though it looks like we might dereference it, so no crash happens in DoesThisCrash1!

void DoesThisCrash2(A* a) {

  SomeObject some_object(a->some_object);

      mov     eax,dword ptr [ebp+8]    eax = a = 0

      push    eax                      First arg for copy constructor

      lea     ecx,[ebp-5]              ecx = this = Location on Stack

      call    SomeObject::SomeObject(copy constructor)

}

Because the copy constructor doesn't do anything with its argument (agreed this would be unusual), we're good to go, so no crash is inherent in DoesThisCrash2!

void DoesThisCrash3(A* a) {

  SomeObject* some_object = a->pointer;

      mov     eax,dword ptr [ebp+8]    eax = a = 0

      mov     ecx,dword ptr [eax+4]    Deref eax+4 = Crash!

      mov     dword ptr [ebp-8],ecx

  some_object->DoNothing();

      mov     ecx,dword ptr [ebp-8]

      call    SomeObject::DoNothing

  a->pointer->DoNothing();

      mov     eax,dword ptr [ebp+8]    eax = a = 0

      mov     ecx,dword ptr [eax+4]    Deref eax+4 = Crash!

      call    SomeObject::DoNothing

}

As you can see, DoesThisCrash3 generates two dereferences for the plain old data A::pointer, crashing at the first dereference for |a|.

Without looking at the definition for the originating type, it isn't always clear what will result in pointer dereference, and what will not.

Additionally, because the behavior of dereferencing a nullptr is undefined, you may or may not crash on other compilers!

A DCHECK would have consistently crashed on a nullptr in each of the functions regardless of compiler behavior and would have found the exact spot of the error instead of later on down the line.

Moral of the story: DCHECK incoming pointers you use if you want to guarantee a crash on nullptr!

Happy Coding!

Robert

[Ilya Sherman]

IMO, if the implementation doesn't actually dereference a null pointer, then it doesn't matter whether the incoming pointer is null.  You might as well just write your code without a pointer being passed at all.  OTOH, if there is a dereference, then it's very unlikely that your code wouldn't crash.  So, I'd personally still prefer to avoid a bunch of redundant DCHECKs for non-nullness all over the codebase...

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

[John Abd-El-Malek]

This seems like a contrived example. In general we pass pointers to objects and we call their methods because they do something.

Adding DCHECKs for all pointers has been an anti-pattern from the beginning of the project, and in practice I haven't seen it hurt us. I worked on a project before that enforced adding CHECKs for pointers in all methods. In practice it was useless since no one monitored the reports from these checks.

[Antoine Labour]

On Wed, Feb 24, 2016 at 2:50 PM, Ilya Sherman <ishe...@chromium.org> wrote:

IMO, if the implementation doesn't actually dereference a null pointer, then it doesn't matter whether the incoming pointer is null.

Counter-example:

void Foo::AddToContainer(Container* container) {

  // |this| can't be nullptr

  container->AddFoo(this);

}

void Container::DoSomethingOnAllFoos() {

  for (Foo* foo : foos_) {

    // |foo| can't be nullptr

    foo->DoSomething();

  }

}

// foo must not be nullptr

void RegisterFoo(Foo* foo) {

  // crash here if foo is nullptr, no need for DCHECK, right?

  foo->AddToContainer(g_container);

}

RegisterFoo(nullptr);  // turns out this doesn't crash

// [...]

g_container->DoSomethingOnAllFoos();  // crash here...

This would cause a crash inside Foo::DoSomething if it does use fields of Foo (or at the callsite if it's virtual). But at this point you have no clue where the nullptr foo comes from.

Instead if you add the DCHECK in AddFoo then you know from the stack trace who was naughty and didn't respect the contract.

This is not a contrived example. Replace Container::AddFoo by AddObserver or PostTask and you'll see how common the pattern can be in our codebase.

  You might as well just write your code without a pointer being passed at all.  OTOH, if there is a dereference, then it's very unlikely that your code wouldn't crash.

What constitutes a dereference in the generated code (as opposed to the source code) is obviously not obvious...

 

  So, I'd personally still prefer to avoid a bunch of redundant DCHECKs for non-nullness all over the codebase...

I personally love DCHECKs. They're your best friend by giving both documentation and a verification opportunity. Use them and love them.

Antoine

 

---
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.

[Ilya Sherman]

On Wed, Feb 24, 2016 at 3:10 PM, Antoine Labour <pi...@google.com> wrote:

On Wed, Feb 24, 2016 at 2:50 PM, Ilya Sherman <ishe...@chromium.org> wrote:

IMO, if the implementation doesn't actually dereference a null pointer, then it doesn't matter whether the incoming pointer is null.

Counter-example:

void Foo::AddToContainer(Container* container) {

  // |this| can't be nullptr

  container->AddFoo(this);

}

void Container::DoSomethingOnAllFoos() {

  for (Foo* foo : foos_) {

    // |foo| can't be nullptr

    foo->DoSomething();

  }

}

// foo must not be nullptr

void RegisterFoo(Foo* foo) {

  // crash here if foo is nullptr, no need for DCHECK, right?

  foo->AddToContainer(g_container);

}

RegisterFoo(nullptr);  // turns out this doesn't crash

// [...]

g_container->DoSomethingOnAllFoos();  // crash here...

This would cause a crash inside Foo::DoSomething if it does use fields of Foo (or at the callsite if it's virtual). But at this point you have no clue where the nullptr foo comes from.

Instead if you add the DCHECK in AddFoo then you know from the stack trace who was naughty and didn't respect the contract.

This is not a contrived example. Replace Container::AddFoo by AddObserver or PostTask and you'll see how common the pattern can be in our codebase.

Agreed, and I do use DCHECKs in this sort of case, where a potentially null pointer is passed into one function, and then dereferenced later.  But IMO that's pretty different than DCHECK(foo) immediately followed by a dereference of foo, as in the original example.

 

  You might as well just write your code without a pointer being passed at all.  OTOH, if there is a dereference, then it's very unlikely that your code wouldn't crash.

What constitutes a dereference in the generated code (as opposed to the source code) is obviously not obvious...

Sure, but does it matter?  Either the generated code crashes, or the DCHECK is irrelevant.

[Fady Samuel]

+1. Storing a pointer in a map or vector or ObserverList etc for later use is super common. Crashing later on use is not particularly useful and entails more debugging to figure out how a null got added in the first place (in other words, adding that DCHECK in the "AddToContainer" method). Rather than going through that while debugging, it seems to make sense to me to keep that in AddToContainer.

Fady

To unsubscribe from this group and stop receiving emails from it, send an email to chromium-dev+unsubscribe@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.

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

[Gabriel Charette]

The advantages of explicit DCHECK for pointers IMO:

1. Explicitly document that the developer knows this pointer can never be null in this code (e.g. when taking a pointer in a map) or explicitly documents the contract of the method.
2. A crash in DCHECK is an obvious tell for the developer writing the code that he did something wrong right there -- instead of a potentially obscure crash requiring debugging.
3. 2. is reinforced by 1. (i.e. otherwise when debugging a crash in 2., even after figuring it out, the developer may inquire whether a nullptr *should* be okay there whereas the DCHECK documents it isn't)

[Antoine Labour]

On Wed, Feb 24, 2016 at 3:15 PM, Ilya Sherman <ishe...@chromium.org> wrote:

On Wed, Feb 24, 2016 at 3:10 PM, Antoine Labour <pi...@google.com> wrote:

On Wed, Feb 24, 2016 at 2:50 PM, Ilya Sherman <ishe...@chromium.org> wrote:

IMO, if the implementation doesn't actually dereference a null pointer, then it doesn't matter whether the incoming pointer is null.

Counter-example:

void Foo::AddToContainer(Container* container) {

  // |this| can't be nullptr

  container->AddFoo(this);

}

void Container::DoSomethingOnAllFoos() {

  for (Foo* foo : foos_) {

    // |foo| can't be nullptr

    foo->DoSomething();

  }

}

// foo must not be nullptr

void RegisterFoo(Foo* foo) {

  // crash here if foo is nullptr, no need for DCHECK, right?

  foo->AddToContainer(g_container);

}

RegisterFoo(nullptr);  // turns out this doesn't crash

// [...]

g_container->DoSomethingOnAllFoos();  // crash here...

This would cause a crash inside Foo::DoSomething if it does use fields of Foo (or at the callsite if it's virtual). But at this point you have no clue where the nullptr foo comes from.

Instead if you add the DCHECK in AddFoo then you know from the stack trace who was naughty and didn't respect the contract.

This is not a contrived example. Replace Container::AddFoo by AddObserver or PostTask and you'll see how common the pattern can be in our codebase.

Agreed, and I do use DCHECKs in this sort of case, where a potentially null pointer is passed into one function, and then dereferenced later.  But IMO that's pretty different than DCHECK(foo) immediately followed by a dereference of foo, as in the original example.

Would you add a DCHECK in RegisterFoo? It sounds like you wouldn't because it's immediately dereferenced.

 

  You might as well just write your code without a pointer being passed at all.  OTOH, if there is a dereference, then it's very unlikely that your code wouldn't crash.

What constitutes a dereference in the generated code (as opposed to the source code) is obviously not obvious...

Sure, but does it matter?  Either the generated code crashes, or the DCHECK is irrelevant.

I think I gave an example where the DCHECK would not be irrelevant (crash with the right stack trace), yet the generated code would not crash.

Antoine

[Ilya Sherman]

I wouldn't add a DCHECK in RegisterFoo(), but I'd add one in AddFoo(), where presumably foo is stored without being dereferenced.  So, RegisterFoo(nullptr) would still reach a DCHECK, just slightly deeper down the stack.

[Sunny Sachanandani]

foo isn't necessarily being dereferenced in RegisterFoo. Are you not adding the DCHECK there because you're assuming that foo is being dereferenced?

[Gabriel Charette]

What if "Container" is third-party code (e.g. STL container) and you can't add a DCHECK in AddFoo()? Would you DCHECK(this) before calling it..?

[John Abd-El-Malek]

On Wed, Feb 24, 2016 at 3:16 PM, 'Fady Samuel' via Chromium-dev <chromi...@chromium.org> wrote:

+1. Storing a pointer in a map or vector or ObserverList etc for later use is super common.

For those cases, the recommendation has been DCHECK right away.

The specific topic at hand is DCHECKing when the pointer is used immediately.

 

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.

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

[Ilya Sherman]

Indeed, if I can't modify AddFoo() directly, then I'd move the DCHECK to immediately precede the call to AddFoo().  FWIW, I think container.Add(this) is much less common than container.Add(foo), where foo is not this.  I admit that DCHECK(this) is somewhat surprising, but honestly so is "DCHECK(foo); foo->DoSomething();".  Both make me do a double-take when reading the code.

[Peter Kasting]

DCHECK is an assertion that a condition never happens.  It is NOT a way to force code to crash on error.  First, since it's DCHECK and not CHECK, it's not compiled into end-user builds, so anything it "forces" is only forced in some of our builds.  Second, and more importantly, DCHECKs should never be used for safety purposes, as that defeats the point of writing the DCHECK just as certainly as handling DCHECK failure does.

Instead, DCHECK should be treated primarily as documentation.  If you need the reader to understand that a condition is intended to always hold at a certain place, you can use DCHECK there.  For example,

void Foo(int* x) {

  DCHECK(x);

  ...

This tells the reader that no code ever calls Foo() with a null pointer.  It does not "force Foo() to crash on null", because the DCHECK means we're not worried about the behavior on null.  It's not intended to be a way of making sure we catch bugs as early as possible; rather, it's a way of making author intent clear so that when we do catch bugs, whenever we catch them, no one has to wonder about whether Foo()'s author simply forgot a null-check.  Would readers of Foo(), in fact, wonder this?  That's the question the author needs to consider.

Thus, make your decision about whether to use DCHECK() based on how much additional clarity you need to provide.  DCHECK can be incredibly useful in making non-obvious preconditions clear, or it can be useless noise in places where the desired precondition is obvious.  Rules about always or never DCHECKing incoming pointers, or anything else, are less good than leaving this to author/reviewer discretion.

PK

[Robert Liao]

Agreed that DCHECK's are only available on DCHECK_IS_ON() builds (and yes, it doesn't actually force a crash), I didn't want to suggest to use CHECK since that would be a little extreme so I took a license on DCHECK since a lot of use run debug builds anyways.

The goal of the message is to clear up some misconceptions about when crashes actually happen on nullptrs. If the code is actually depending on a crash to point out invalid state (e.g. the caller incorrectly passed in nullptr), it is insufficient to simply have the code perform what looks like a dereference and call it a day. 

In the code review I mentioned above, I thought I found a case where a DCHECK was redundant (pointer dereference), but it turned out the code didn't crash on a nullptr! The investigation into that strange behavior led to the above discussion where the base pointer itself wasn't actually dereferenced, even though from code inspection it looked like it should have been.

While DCHECKs are typically used for documentation, it has the impact of catching bugs sooner or in a better spot. DCHECKing before adding to an collection of pointers is a well-agreed practice that most of us already do.

[Peter Kasting]

On Wed, Feb 24, 2016 at 4:39 PM, Robert Liao <rob...@chromium.org> wrote:

The goal of the message is to clear up some misconceptions about when crashes actually happen on nullptrs. If the code is actually depending on a crash to point out invalid state (e.g. the caller incorrectly passed in nullptr), it is insufficient to simply have the code perform what looks like a dereference and call it a day. 

Agreed.

In the code review I mentioned above, I thought I found a case where a DCHECK was redundant (pointer dereference), but it turned out the code didn't crash on a nullptr!

That's sort of what I mean: DCHECKs are never redundant with pointer dereferences, because the pointer deref tells you the coder wanted the contents of that memory location, but the DCHECK tells you that a null pointer check wasn't forgotten and isn't necessary because the caller should have guaranteed this would be non-null.

This is why in many places DCHECKs aren't necessary before mucking with pointers: the scope of the control flow is limited enough (e.g. helper functions within a class) that it's obvious a pointer won't be null, or the API makes no conceivable sense to use with null (e.g. View::AddChildView()).  In such cases the DCHECK tells you no information you didn't already have.

If you're trying to temporarily track down misuse of an API and want to crash sooner, use CHECK rather than DCHECK, and remove the CHECKs once the problem is gone.  If you insert permanent DCHECKs that tell the coder nothing beyond what was already apparent, it's like inserting comments that restate the code; it decreases readability, and the potential gain of maybe catching potential misuse sooner someday is generally not worth the cost.

Because this is a subjective readability issue, this will ultimately be up to author and reviewer; but I would say, avoid DCHECKs in places where the precondition stated is already blindingly obvious.  I definitely don't support rules like "always DCHECK pointers going into a container" any more than I support "never DCHECK such pointers".

PK

[Nico Weber]

On Wed, Feb 24, 2016 at 8:26 PM, Peter Kasting <pkas...@chromium.org> wrote:

On Wed, Feb 24, 2016 at 4:39 PM, Robert Liao <rob...@chromium.org> wrote:

The goal of the message is to clear up some misconceptions about when crashes actually happen on nullptrs. If the code is actually depending on a crash to point out invalid state (e.g. the caller incorrectly passed in nullptr), it is insufficient to simply have the code perform what looks like a dereference and call it a day. 

Agreed.

In the code review I mentioned above, I thought I found a case where a DCHECK was redundant (pointer dereference), but it turned out the code didn't crash on a nullptr!

That's sort of what I mean: DCHECKs are never redundant with pointer dereferences, because the pointer deref tells you the coder wanted the contents of that memory location, but the DCHECK tells you that a null pointer check wasn't forgotten and isn't necessary because the caller should have guaranteed this would be non-null.

This is why in many places DCHECKs aren't necessary before mucking with pointers: the scope of the control flow is limited enough (e.g. helper functions within a class) that it's obvious a pointer won't be null, or the API makes no conceivable sense to use with null (e.g. View::AddChildView()).  In such cases the DCHECK tells you no information you didn't already have.

If you're trying to temporarily track down misuse of an API and want to crash sooner, use CHECK rather than DCHECK, and remove the CHECKs once the problem is gone.  If you insert permanent DCHECKs that tell the coder nothing beyond what was already apparent, it's like inserting comments that restate the code; it decreases readability, and the potential gain of maybe catching potential misuse sooner someday is generally not worth the cost.

Because this is a subjective readability issue, this will ultimately be up to author and reviewer

+1, no reason to be dogmatic about this, use your best judgement.

In general, calling objects on null objects is undefined behavior, which means it'll eventually be caught by an UBSan bot as long as your code has test coverage. (We don't have a working UBSan bot yet as far as I know, though). So there's no need to DCHECK every pointer in general, but if you're going to store it somewhere or something, DCHECK when you store it (or do whatever else your best judgement tells you) – but have test coverage :-)

 

; but I would say, avoid DCHECKs in places where the precondition stated is already blindingly obvious.  I definitely don't support rules like "always DCHECK pointers going into a container" any more than I support "never DCHECK such pointers".

PK

--

[Antoine Labour]

On Wed, Feb 24, 2016 at 5:39 PM, Nico Weber <tha...@chromium.org> wrote:

On Wed, Feb 24, 2016 at 8:26 PM, Peter Kasting <pkas...@chromium.org> wrote:

On Wed, Feb 24, 2016 at 4:39 PM, Robert Liao <rob...@chromium.org> wrote:

The goal of the message is to clear up some misconceptions about when crashes actually happen on nullptrs. If the code is actually depending on a crash to point out invalid state (e.g. the caller incorrectly passed in nullptr), it is insufficient to simply have the code perform what looks like a dereference and call it a day. 

Agreed.

In the code review I mentioned above, I thought I found a case where a DCHECK was redundant (pointer dereference), but it turned out the code didn't crash on a nullptr!

That's sort of what I mean: DCHECKs are never redundant with pointer dereferences, because the pointer deref tells you the coder wanted the contents of that memory location, but the DCHECK tells you that a null pointer check wasn't forgotten and isn't necessary because the caller should have guaranteed this would be non-null.

This is why in many places DCHECKs aren't necessary before mucking with pointers: the scope of the control flow is limited enough (e.g. helper functions within a class) that it's obvious a pointer won't be null, or the API makes no conceivable sense to use with null (e.g. View::AddChildView()).  In such cases the DCHECK tells you no information you didn't already have.

If you're trying to temporarily track down misuse of an API and want to crash sooner, use CHECK rather than DCHECK, and remove the CHECKs once the problem is gone.  If you insert permanent DCHECKs that tell the coder nothing beyond what was already apparent, it's like inserting comments that restate the code; it decreases readability, and the potential gain of maybe catching potential misuse sooner someday is generally not worth the cost.

Because this is a subjective readability issue, this will ultimately be up to author and reviewer

+1, no reason to be dogmatic about this, use your best judgement.

In general, calling objects on null objects is undefined behavior, which means it'll eventually be caught by an UBSan bot as long as your code has test coverage. (We don't have a working UBSan bot yet as far as I know, though). So there's no need to DCHECK every pointer in general, but if you're going to store it somewhere or something, DCHECK when you store it (or do whatever else your best judgement tells you) – but have test coverage :-)

+1 on test coverage.

Adding DCHECKs may seem redundant sometimes, but they're also very useful to document contracts, in a form that will:

- survive refactoring (the explicit dereference may end up 3 functions deep, in another file, etc.)

- pop up on bots when the contract is not fulfilled, with a clear(er) error message than a crash

- hopefully be closer to the source of the mistake, so that it's easier to determine the cause - e.g. place yourself in the shoes of a sheriff, who knows nothing about Foo, RegisterFoo, Container::AddFoo... If an assert pops in RegisterFoo and a patch changed the RegisterFoo caller, that's a clear suspect. It gets harder as the DCHECK is further from the source (deep down in Container::AddFoo).

Also keep in mind that DCHECK(this) is DCHECK(true), and a compiler may just remove it altogether as dead code. In fact, any DCHECK that would only trigger after the damage is done (NULL deref) might just be eliminated by an infinitely smart compiler.

Anyway, all that to say, let's not be dogmatic about "never DCHECK before a deref", use your best judgement indeed.

Antoine

 

 

; but I would say, avoid DCHECKs in places where the precondition stated is already blindingly obvious.  I definitely don't support rules like "always DCHECK pointers going into a container" any more than I support "never DCHECK such pointers".

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

--
--
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, Feb 25, 2016 at 2:56 PM, Antoine Labour <pi...@google.com> wrote:

Anyway, all that to say, let's not be dogmatic about "never DCHECK before a deref"

I haven't seen anyone on this thread argue for that.  If you're referring to my comments with this, I've been wildly misconstrued.  (The whole point of saying "DCHECKs are never redundant with pointer dereferences" was to note that because the two mean different things, you can't just assume a deref means there's no need for a DCHECK.)

PK 

[Brett Wilson]

I've been the source of some of this DCHECK push-back. I do usually strongly recommend not to DCHECK before a deref unless there's a good reason to do so. There are certainly good reasons to do so in some cases, especially things like AddFoo in Antoine's example.

But there's a certain style of coding that I think we should be staying away from that with optional pointers all over the place. People insert null checks and assertions everywhere because they have no idea what's going on. Adding checks "just in case" because you dereference a pointer mostly adds noise, doesn't usually increase debugability, and I think encourages the general feeling of "pointers are optional."

Brett

[Antoine Labour]

I wholeheartedly agree, and this is a typical case where I push people to replace a if (foo_) foo_->Blah(); into a DCHECK(foo_); foo_->Blah();

The DCHECK indicates that, yes, someone thought about this carefully, and no, you can't add a if (foo_) because you have a crash here that you don't understand - go back and trace why this is called when the pointer is NULL in the first place, and fix that.

Antoine

 

Brett

[Peter Kasting]

On Thu, Feb 25, 2016 at 3:15 PM, Brett Wilson <bre...@chromium.org> wrote:

But there's a certain style of coding that I think we should be staying away from that with optional pointers all over the place. People insert null checks and assertions everywhere because they have no idea what's going on. Adding checks "just in case" because you dereference a pointer mostly adds noise, doesn't usually increase debugability, and I think encourages the general feeling of "pointers are optional."

Yes, inserting a null check when you're not sure whether it's necessary/how to trigger it is much, much worse than either including or omitting DCHECKs erroneously.  It implies that the pointer can be null there, which leads future maintainers and anyone calling or called by the code to assume they also need to handle it.

If there's any doubt about whether a pointer can be null, authors should go back and find a way to trigger it being null, and document that ("can be null in tests", for example), or else avoid the conditional and, likely, insert a DCHECK.

PK