11 ms·
That code is not a general purpose library function; it assumes the element exists. If you call that method and the element doesn't exist, presumably something
by yarcob 6y ago
That code is not a general purpose library function; it assumes the element exists.
If you call that method and the element doesn't exist, presumably something has gone wrong already. Adding a NULL pointer check is not going to fix it. You just silently ignore the error. You'll prevent the crash, but there's no mechanism for handling the error.
- adrianN 6y agoAnd instead of adding a check and crashing visibly here you wait even longer until your program misbehaves due to the error? The longer the program runs after something went wrong, the harder it is to debug, and the more likely it becomes that you get an exploitable bug.
- larsrc 6y agoAre you arguing that we should leave known crash bugs in the code?
- aidos 6y agoIt’s not a bug, in this case. It depends on the behaviour the library is specified to handle, but if you ask me to remove a specific item from a list, and that item isn’t in the list, what should I do? I can fail so you can catch it, or if you don’t catch it, you’ll at least see exactly what went wrong. You expected the item to be in the list and it wasn’t. Your preconditions were wrong from the beginning and you have an (unknown) bug in the code. The alternative is that I can’t find the item and I silently suck it up and don’t notify you. If that’s the api we’ve all agreed on, that’s fine. It was on you to check for the thing in the list first and then remove it. But it’s a bit weird if you think about it - now you need to traverse the list twice. Once to check for the existence (and fail out yourself) and once to remove.
- christophilus 6y agoI think a better approach is to return a bool or something like that, rather than crashing. But I agree that silently handling this case is the worst of the options.
- adrianN 6y agoI'm arguing that we should use asserts or similar defensive coding mechanisms to ensure that preconditions are met instead of merrily continuing to run our program with cascading error effects.
- BoorishBears 6y agoI've had this argument a ton with people used to new high level languages that make null safety "too easy" like: next?.doThing() Where doThing is never called if next is null. Languages that do this use "scary" operators to crash on null: next!!.doThing() And it's drilled into people's heads the latter is a Bad Thing (tm) - You really need to consider context in null handling. Imagine an app that alerts a nurse when the patients heartbeat is out of range. In an application where the UI context might have been closed out, it's common to see someUiContext?.showAlert() But what actually happens if the context is gone? It's better to crash and have part of your startup procedure be communicating that a crash occurred, and the doctor should check that something went wrong, than silently continuing. - The problem is when you tell people this, the kneejerk reaction is always "are you saying we intentionally add crashes"! Because safe null handling was specifically added to avoid the situation where that crashes... (For example, if the app was a news reader and the alert was "Article failed to load", you wouldn't want the app to crash just because the user left a certain page before the alert was shown) But I think the pendulum has swung too far at this point, people are so used to just sweeping nulls under the rug, and it's not great for finding issues
- veets 6y agoYes - and that is the point the OP you responded to is making. This is not a generic library function. It used in a specific setting where preconditions exist and presumably are checked _prior_ to calling this code. Sure you could make the argument that we don't _know_ they are being checked, but it's a pointless discussion. Who cares? _If_ preconditions are met, this code is safe, if they aren't, it's not safe. Since we don't know one way or the other, there's no point in discussing further. The kernel developers know their stuff...
- adrianN 6y ago
- banachtarski 6y agoWhat you are espousing is known as “defensive programming” which is very much a bad thing (tm).
- adrianN 6y agoI'm interested in hearing more about why defensive programming is bad. Do you have some links for me?
- stuff4ben 6y agoClearly you've never worked with contract developers whose only purpose is to write a crap ton of code to get a "feature" out that crashes as soon as it encounters the real world. As they say, "doveryay, no proveryay" and it's served me well.
- hyperdimension 6y ago> As they say, "doveryay, no proveryay" I'm sorry if this might be common knowledge, but I've honestly never heard this and can't understand it. Is it saying that it's better to do something than to (formally) prove it, like the opposite of (I think) Knuth's famous quote?
- DanBC 6y ago"trust, but verify". https://en.wikipedia.org/wiki/Trust,_but_verify https://en.wikipedia.org/wiki/Trust,_but_verify
- snarfy 6y agoPerformance is a feature and that null check has a cost. The code is correct. The null check is unnecessary. The bug is passing null in.
- stuff4ben 6y agoSafety is a feature and that null check prevents bugs. I'd rather have slow but safe code rather than fast but buggy code.
- pmart123 6y agoAdding checks creates more code branches and tests. Passing null in is the bug.
- freemint 6y agoCan you show it has any impact on benchmarks? This is something speculative execution should take care of that quickly? "Premature optimization.... " you get the drift.
- Sohcahtoa82 6y agoIf the null check silently ignores the attempt to remove a non-existent item from the list, then the null check did not prevent a bug, it merely hid it. IMO, even though this is C, the Zen of Python still applies, which states: > Errors should never pass silently unless explicitly silenced. Its the caller with the bug, not the library code.
- ninkendo 6y agoCan we all at least agree that if the language could express non-nullable pointers, we'd all be better off? (Something like `Option<T>` in Rust, or Swift's `?` sugar for their `Optional`, etc etc) Then the type of the function would just declare a that the pointer can't be null, and code which sends a nullable pointer would refuse to compile.
- 6y ago
- scythe 6y agoWhere you add safety checks like that is a top-down system organization issue, not something you can couch quarterback while looking at twelve lines from a million-line codebase. There is simply no way to determine if an assert is appropriate without further context, and it has nothing to do with the choice of algorithm.
- JulianMorrison 6y agoThen it ought to document that it assumes that entry exists and that this walk will not hit the end of the list. Which also implies the list is not empty.
- Cthulhu_ 6y agoIt should, yeah, but if it can safely assume there will be no nulls, it will save a nontrivial amount of defensive programming checks for each cycle. And IIRC, the Linux kernel uses tons of linked lists, millions of items per second sometimes; any operation that can be removed will make a big impact. Relatively. But for general purpose code, e.g. libraries where performance isn't super important but stability is, more defensive programming would be a thing to do.