8 ms·

Ahhh, so it was a problem with lazy evaluation (and the lack thereof when using &), and not type coercion, like I initially thought.

by fouric 5y ago

dataflow 5y ago
Yeah it has nothing to do with type coercion. I'm baffled a linter doesn't catch this though. & for bools should just be prohibited altogether. If people really want & for bool, they can just cast to an integer type and use & with that (which generates its own warnings appropriately).
- gpderetta 5y ago
  the problem is that bitwise & and | are routinely used for their non-shortcircuiting behaviour in boolean expressions, so it is hard for the compiler to flag this as an error. Many linters and static analysis tools do flag them though as these days it is not considered good practice.
  - dataflow 5y ago
    > & and | are routinely used for their non-shortcircuiting behaviour in boolean expressions Really? I can't recall a single case of this with 'bool' in any codebase in my recent memory. I only see it with integers. And even if it's somehow routine for your codebases with bool (why?!), surely it's not routine inside a conditional expression, so at least they can prohibit it there?
    - kevin_thibedeau 5y ago
      Whenever I see it I assume it's a typo. If it was truly intentional I would expect a comment pointing out why it's needed.
  - anyfoo 5y ago
    Why would you even need non-shortcircuiting behavior in boolean expressions? Because either side of the operator has side effects that you want to happen unconditionally? Please just write it out as an extra statement then instead of hammering it into place with an implicit coercion to an integer type only so you can abuse bitwise AND and OR only to force that side effect to... you can see why this is probably not the most elegant idea.
    - slavik81 5y ago
      Matt Godbolt gave an example of a significant performance hit caused by unnecessary short-circuiting in his CppCon 2019 talk[1]. EDIT: Deleted tangent. [1]: https://youtu.be/HG6c4Kwbv4I?t=45m https://youtu.be/HG6c4Kwbv4I?t=45m
      - thaumasiotes 5y ago
        This example is interesting enough that it deserves to be written up for convenience. The reason there is a performance hit is that the example involves a lot of mispredicted branches. The setup is that we have a large number of tiny triangles, we cast a ray in a random direction, and we want to detect whether it intersects any of the triangles. The summarized code looks like this: for (/* all triangles */) { auto u = calcU(/*...*/); if (u < 0 || u > 1) { continue; } auto v = calcV(/*...*/); if (v < 0 || u + v > 1) { continue; } auto dist = calcD(/*...*/); if (dist < nearest) { nearest = dist; } } So what's happening? We calculate the (x,y) coordinates of the point where our ray intersects the plane in which the triangle lies. We convert those (x,y) coordinates to (u,v) coordinates, where the vectors u and v are parallel to two sides of the triangle. (And equal in length.) In our transformed (u,v) space, determining whether a point lies inside the triangle is very easy. With the origin at the corner of the triangle from which the u and v sides emanate, a point is out of bounds if its u-coordinate lies outside the interval [0, 1], or if its v-coordinate lies outside [0, 1-u]. That's what the ifs after calcU and calcV are checking. When we detect that a point is out of bounds, we move on to the next triangle. The triangles are small and the ray is random, so the intersection of the ray with the plane will strike at a random point. It is almost always the case that the point will lie outside the triangle. This means that the full conditional (u < 0 || u > 1) will almost always be false. But the two subconditions u < 0 and u > 1 will each be true 50% of the time. They are individually impossible to predict, which causes branch prediction on the first one, u < 0, to fail about 50% of the time. But they are jointly easy to predict. There is a huge performance gain from switching to (u < 0 | u > 1) -- in this case, 50% of the time we will do the work of calculating whether u > 1 even though we didn't have to. But the reward we get for that extra work is that branch prediction drops from a 50% failure rate (actually 45%) to a very low failure rate. Including the v-coordinate makes the full check, ((u < 0) | (u > 1) | (v < 0) | (u + v > 1)), even more easy to predict. We've decided to guarantee that we will always do the full amount of work, and most of it is unnecessary. But it's easier to do 2x or more the amount of work and test a condition that fails consistently than to do less work and keep having to clear the instruction pipeline.
kbelder 5y ago
I think this is a unpopular opinion, but I consider taking advantage of short-circuiting in boolean operations to be a code smell. At least for me, it seems very fragile and I'd much rather break the conditional logic out explicitly. But I like to write as much like assembly as I can in every language. Each line of code should do one and only one thing.
- dataflow 5y ago
  I used to only put short-circuits when I really needed short-circuit behavior, thinking it improved readability. But after getting by the implicit type conversions over and over (as well as nags from every compiler) I just decided it was a bad idea in C++. I'd still do it in a safer language, but in C++ I'm convinced you should just use && even when you only need & for bools.
- saagarjha 5y ago
  Short circuiting boolean operators are very useful, and not something I would consider a code smell. They let constructs like this work, which are very common in most codebases: if (foo && foo->bar) { // whatever } An operator that did not short circuit would have undefined behavior because it might dereference a NULL pointer.
  - JonathonW 5y ago
    If you're going for "each line of code should do exactly one thing", you'd probably prefer that as if(foo) { if(foo->bar) { //whatever } } separating out the null check and the actual conditional. More lines of code and more nesting, yes, but, if you're trying to strictly adhere to a one thing/one line principle, you probably don't care. Short-circuit 'or' is a little harder to avoid (if you specifically want the short-circuit behavior), since you'd have to duplicate the code in the body of the if. But that doesn't come up as often IME.
    - kbelder 5y ago
      Yep, I'd prefer that option, despite its wordiness. Obviously this depends a lot on individual preferences, and in this simple example it really doesn't make a lot of difference. I imagine the compiler spits out pretty similar code either way. It's more just a recognition of my own limitations... I'm a lot less likely to screw up something up, the more explicitly it's written out. When I was a younger man, I wouldn't have bothered.

Ahhh, so it was a problem with lazy evaluation (and the lack thereof when using &), and not type coercion, like I initially thought.

by fouric 5y ago

Ahhh, so it was a problem with lazy evaluation (and the lack thereof when using &), and not type coercion, like I initially thought.

dataflow 5y ago

Yeah it has nothing to do with type coercion. I'm baffled a linter doesn't catch this though. & for bools should just be prohibited altogether. If people really want & for bool, they can just cast to an integer type and use & with that (which generates its own warnings appropriately).

gpderetta 5y ago
the problem is that bitwise & and | are routinely used for their non-shortcircuiting behaviour in boolean expressions, so it is hard for the compiler to flag this as an error. Many linters and static analysis tools do flag them though as these days it is not considered good practice.
- dataflow 5y ago
  > & and | are routinely used for their non-shortcircuiting behaviour in boolean expressions Really? I can't recall a single case of this with 'bool' in any codebase in my recent memory. I only see it with integers. And even if it's somehow routine for your codebases with bool (why?!), surely it's not routine inside a conditional expression, so at least they can prohibit it there?
  - kevin_thibedeau 5y ago
    Whenever I see it I assume it's a typo. If it was truly intentional I would expect a comment pointing out why it's needed.
- anyfoo 5y ago
  Why would you even need non-shortcircuiting behavior in boolean expressions? Because either side of the operator has side effects that you want to happen unconditionally? Please just write it out as an extra statement then instead of hammering it into place with an implicit coercion to an integer type only so you can abuse bitwise AND and OR only to force that side effect to... you can see why this is probably not the most elegant idea.
  - slavik81 5y ago
    Matt Godbolt gave an example of a significant performance hit caused by unnecessary short-circuiting in his CppCon 2019 talk[1]. EDIT: Deleted tangent. [1]: https://youtu.be/HG6c4Kwbv4I?t=45m https://youtu.be/HG6c4Kwbv4I?t=45m
    - thaumasiotes 5y ago
      This example is interesting enough that it deserves to be written up for convenience. The reason there is a performance hit is that the example involves a lot of mispredicted branches. The setup is that we have a large number of tiny triangles, we cast a ray in a random direction, and we want to detect whether it intersects any of the triangles. The summarized code looks like this: for (/* all triangles */) { auto u = calcU(/*...*/); if (u < 0 || u > 1) { continue; } auto v = calcV(/*...*/); if (v < 0 || u + v > 1) { continue; } auto dist = calcD(/*...*/); if (dist < nearest) { nearest = dist; } } So what's happening? We calculate the (x,y) coordinates of the point where our ray intersects the plane in which the triangle lies. We convert those (x,y) coordinates to (u,v) coordinates, where the vectors u and v are parallel to two sides of the triangle. (And equal in length.) In our transformed (u,v) space, determining whether a point lies inside the triangle is very easy. With the origin at the corner of the triangle from which the u and v sides emanate, a point is out of bounds if its u-coordinate lies outside the interval [0, 1], or if its v-coordinate lies outside [0, 1-u]. That's what the ifs after calcU and calcV are checking. When we detect that a point is out of bounds, we move on to the next triangle. The triangles are small and the ray is random, so the intersection of the ray with the plane will strike at a random point. It is almost always the case that the point will lie outside the triangle. This means that the full conditional (u < 0 || u > 1) will almost always be false. But the two subconditions u < 0 and u > 1 will each be true 50% of the time. They are individually impossible to predict, which causes branch prediction on the first one, u < 0, to fail about 50% of the time. But they are jointly easy to predict. There is a huge performance gain from switching to (u < 0 | u > 1) -- in this case, 50% of the time we will do the work of calculating whether u > 1 even though we didn't have to. But the reward we get for that extra work is that branch prediction drops from a 50% failure rate (actually 45%) to a very low failure rate. Including the v-coordinate makes the full check, ((u < 0) | (u > 1) | (v < 0) | (u + v > 1)), even more easy to predict. We've decided to guarantee that we will always do the full amount of work, and most of it is unnecessary. But it's easier to do 2x or more the amount of work and test a condition that fails consistently than to do less work and keep having to clear the instruction pipeline.

kbelder 5y ago

I think this is a unpopular opinion, but I consider taking advantage of short-circuiting in boolean operations to be a code smell. At least for me, it seems very fragile and I'd much rather break the conditional logic out explicitly. But I like to write as much like assembly as I can in every language. Each line of code should do one and only one thing.

dataflow 5y ago
I used to only put short-circuits when I really needed short-circuit behavior, thinking it improved readability. But after getting by the implicit type conversions over and over (as well as nags from every compiler) I just decided it was a bad idea in C++. I'd still do it in a safer language, but in C++ I'm convinced you should just use && even when you only need & for bools.
saagarjha 5y ago
Short circuiting boolean operators are very useful, and not something I would consider a code smell. They let constructs like this work, which are very common in most codebases: if (foo && foo->bar) { // whatever } An operator that did not short circuit would have undefined behavior because it might dereference a NULL pointer.
- JonathonW 5y ago
  If you're going for "each line of code should do exactly one thing", you'd probably prefer that as if(foo) { if(foo->bar) { //whatever } } separating out the null check and the actual conditional. More lines of code and more nesting, yes, but, if you're trying to strictly adhere to a one thing/one line principle, you probably don't care. Short-circuit 'or' is a little harder to avoid (if you specifically want the short-circuit behavior), since you'd have to duplicate the code in the body of the if. But that doesn't come up as often IME.
  - kbelder 5y ago
    Yep, I'd prefer that option, despite its wordiness. Obviously this depends a lot on individual preferences, and in this simple example it really doesn't make a lot of difference. I imagine the compiler spits out pretty similar code either way. It's more just a recognition of my own limitations... I'm a lot less likely to screw up something up, the more explicitly it's written out. When I was a younger man, I wouldn't have bothered.