Hacker Newsnew | past | comments | ask | show | jobs | submitlogin

  [regexp] Remove trivial assertion

  The assertion in BytecodeSequenceNode::ArgumentMapping cannot fail,
  because size_t is an unsigned type. This triggered static analysis
  warnings in SpiderMonkey.
Does that mean Chromium doesn't use any static analysis tool? Or one that does not work for this trivial assertion?


> Or one that does not work for this trivial assertion?

Or one that does work for this trivial assertion, which is to say not emit a diagnostic.

What ends up happening in a situation like this is that somebody removes the assertion to quiet the compiler diagnostic. Then the types change and all of a sudden it can fail, but the assertion is gone.

You might say, "well, now that one of the operands is size_t how could the types ever change to reintroduce an issue?" That's the wrong question to ask. The whole point of the assertion is to avoid having to answer such a tricky question, or at least to encode your answer in a way that if the unforeseen happens things break loudly instead of silently. Anyhow, you'd be surprised by how things can break. I personally religiously use size_t for anything related to object size, but many other developers don't (including at Google and Mozilla), and so you often see a mix of, e.g., size_t and uint64_t, size_t and uint32_t, size_t and int, etc, and regular tweaks back-and-forth, which can easily introduce regressions.

I understand why compilers emit a warning--the idea is that if the assertion couldn't possibly be false, maybe it has a bug. But, IME, the opposite is usually true--it's well-written and deliberate, because the developer is trying to catch spooky action at a distance where the type, which is defined far away, is changed, accidentally or intentionally. I don't know where to draw the line in terms of second-guessing the code to help catch bugs, but GCC and clang need to provide far more succinct constructs to tell the compiler to shut up. Currently you're stuck with inline pragmas, __extension__, statement expressions, and other weird convolutions that require far more code than the assertion or operation itself. The issue makes writing arithmetic overflow-safe code more tedious and error prone.

(Other languages simply prohibit mixing integer operands of different types, so if a type is changed far away then code will break loudly even without any assertions. But in codebases like SpiderMonkey and V8 that use a wide range of integer types for space and performance optimizations, that tends to encourage casting, which has the exact same problems.)


But the bug is not in that function, if a function accepts size_t, then that value can never be <0. If someone were to pass (size_t)-1 to it, it would still be positive. The issue is in the caller of the function, which must convert a negative value of a signed type into size_t, and that is the actual bug in this situation. Perhaps those conversions should trigger at least a compiler warning.

Asserting things which are given from the types is a waste. Or do you think we should assert that a value of type bool is really either true or false and fail the program otherwise?




Guidelines | FAQ | Lists | API | Security | Legal | Apply to YC | Contact

Search: