• Jon Skeet (unregistered)

    First to have a million rep!

  • Me (unregistered)

    TRWTF is to overload well-understood operators. But then again, I'm more of a Knopfler than a van Halen.

  • (nodebb)

    Cargo cult at its best, although unusually the cargo cult was accurate for an unexpectable reason.

    Then again, my company's codebase just suffered a "change all files" kind of thing, and it broke some creaky code that I'm responsible for.

    OK, we won't discuss the commit that got pushed without proper testing, although that's a WTF in its own right, because it's not the WTF I want to talk about. I fixed five instances of a different problem, where someone thought it was a good idea to write this code to compare two integers to see if one is greater than the other:

        if ( (a - b) > 0 )

    which works fine if both a and b are signed. The "change all files" commit was there to fix all the -Werror warnings generated by activating "sign-compare" warnings (long story), and both a and b became unsigned, so the difference is always either zero or greater than zero.

    It took a while to find this, but I eventually noted a different problem which meant that at point X in the code, awas set to zero, while b should have been, but wasn't (and wasn't zero either). In reality, the condition wasn't relevant in this specific case, but the meaning of the if became "if a and b are different" instead of "if a is greater than b"...

    I replaced the five instances with:

        if ( a > b )

    and the automated regression test(1) now passes.

    (1) That's how we know that a breaking change had been committed to the master branch, but it would have been nice if they had run a manual round before committing.

  • Prime Mover (unregistered) in reply to Me

    TRWTF is to use a language which allows such appallingly dangerous practice as to overload well-understood operators in the first place, but then I'm more of a Gilmour than a Knopfler.

  • Charles (unregistered)

    In this case Eddie was partly right and Lee definitely wrong. A null value for a coordinate would mean something like no coordinate is known or none is relevant or some similar situation. Two such values are not equal because, for instance, if place A is at an unknown location and place B is at an unknown location that does not mean they're at the same location. Adding the possibility of null means that the == and != operators have three possible values: true, false, and unknown. Eddie was right to have made the change, and unfortunately missed the if (p != null). However, in writing the overloaded != operator, he failed to handle null correctly. It probably should throw an exception (or, even better, be asserted not to be present) as it is impossible to tell whether the coordinates are equal, so it should not confidently return a value in all cases.

  • Charles (unregistered) in reply to Steve_The_Cynic

    (a - b) > 0 does not work fine if both are signed (I'm assuming a language such as C or C++ because of the refernce to -Weror) because the subtraction can overflow, leading to undefined behaviour. However a > b is always correct.

  • (nodebb)

    Why didn't the unit tests for the equality operator fail even before Eddie could generate the change request?????

    [Rhetorical]

  • (nodebb) in reply to Charles

    Good point, although in the case here, neither value can get far enough from zero to cause problems - both are limited to a few tens of thousands at the most, while a and b are both 32-bit (changed to 32/64 size_t in the unsigned version).

    Addendum 2020-12-03 07:59: And also they are strictly non-negative (indexes into a buffer), so subtraction can't overflow anyway.

  • Paul (unregistered)

    This is ... the complete opposite of a WTF.

    Someone wanted to make a change that would prevent a bug. The argument against it was "we don't have that bug yet, don't rock the boat"? No actual reasons as to why it was worse to make this change mind you... Then, when the bug actually crops up, that's a reason to mock the person who wanted to make the change?

    There are valid reasons to add custom logic to the "equals" on a class, but the surprise factor probably means you should do something else. But THAT change got through just fine while they wanted to argue about a simple refactor.

  • fa (unregistered)

    If the original code before Eddie was:

    !(pt1 == null) && !(pt2 == null)

    woudn't this be an infinite recursion, as it would call the equality operator again?

  • Brian (unregistered)

    I'm with the "needs more unit testing" camp. There are perfectly valid reasons for operator overloading (for example, turning a default reference compare into a value compare)... but it is admittedly dangerous and should be unit-tested to death to prove that the overloads behave as expected in all possible cases, including (especially) how it handles nulls. Then Eddie's argument is moot.

    Also, global find-and-replace on all files is almost always the wrong approach, even if you're "clever" enough to do it with regex.

  • WTFGuy (unregistered)

    @Charles ref

    In this case Eddie was partly right and Lee definitely wrong. A null value for a coordinate would mean something like no coordinate is known or none is relevant or some similar situation. Two such values are not equal because, for instance, if place A is at an unknown location and place B is at an unknown location that does not mean they're at the same location. Adding the possibility of null means that the == and != operators have three possible values: true, false, and unknown. Eddie was right to have made the change, and unfortunately missed the if (p != null). However, in writing the overloaded != operator, he failed to handle null correctly. It probably should throw an exception (or, even better, be asserted not to be present) as it is impossible to tell whether the coordinates are equal, so it should not confidently return a value in all cases.

    As a matter of philosophy I don't disagree with your overarching point. But any time you create a situation where the value of (a==b) vs the value of !(a!=b) can differ, you set up to break one heck of a lot of assumptions in the .Net Framework. It's a darn shame that back in v1.0 they didn't take the POV that null is simply not comparable, and any comparison operator applied to null yields an e.g. NullCompareException. The is and is not operators should be only ones defined on null.

    But we are where we are and by the standard, null == null.

  • ZZartin (unregistered)

    Or just don't overload operators.... you know for this exact reason.... guess he missed that in his best practices.

  • (nodebb)

    The problem here is they used operator overloading for the wrong purpose. That equality belongs in the equals override. And yes he missed the negative is re factoring

  • (nodebb)

    WRT the language itself, equality operator and equals methods should never have been defined on object class to begin with. The ideal approach would've been, equals method automatically creates equality operator for that type only, and the operator can be custom overloaded for very edge cases, think custom expression tree creation.

  • true rockstar (unregistered)

    TRWTF is rockstar wannabes using operator overloading. Overloading is great, I once worked with the code which had a comma (,) overloaded. Fun times.

  • Laie Techie (unregistered)

    Java has ==, ===, and equals with slightly different means of "equal". === checks for identity, which is to say, are they located at the same location in memory. == looks for equality, but can do some conversion in the process (eg. convert an int to a double). Only the equals function can be overwritten. The equals function should check for the values of any pertinant fields. If the equals function is overwritten, so should hashCode.

    I think equality generally depends on a context. Numbers might be rounded to have the same number of significant digits before comparing, for example.

  • tbo (unregistered) in reply to Charles

    Whether "no location" is equal to "no location" greatly depends on what you mean by "equal to" and "no location."

  • cellocgw (unregistered) in reply to WTFGuy
    But we are where we are and by the standard, null == null.

    Well, that scared me, so I checked my fave data language, "R" . comparing NULL with anything (number or another NULL) always returns nothing at all, i.e. the comparison operators refuse to "test" NULL. Thank goodness for that.

  • MiserableOldGit (unregistered) in reply to Charles

    I'd say ascribing multiple meanings to null that would direct program flow is a bigger WTF than overloading the standard operator in a dumb-ass way.

    Null may arise for multiple reasons, but it should never be used to specifically mean something beyond "Umm, I dunno?" because you should never really assume your plan for a variable is the only way it could reach that test with a null in it.

    Null == null is true because maths and logic say so, any attempt to code around that (other than making it raise an exception) is serious smell. But equally (geddit?) relying on that as part of normal program flow, rather than exception states is just asking for trouble.

  • (nodebb) in reply to fa

    Yeah, I tried to make sure. The original version couldn't have been "==" Something else changed here.

  • Dyspeptic Curmudgeon (google)

    My frist thought on reading that is that the WTF is using !p==null when dealing with a location. Should be the other way round: ' if p==NULL -> fail'' A location is either set or not (presumably here, NULL if not set as 0.0, 0.0 is a real location). And you are only (at this point) interested in determining whether you have allowable input. Which also points to the fact that this code clearly does not have proper data cleaning or correction at the point of entry since this testing is being done here.

  • Naomi (unregistered)

    Okay, I'm just going to say it: TRRWTF is cargo-cult declarations that an obvious, useful implementation of the equality operator is TRWTF.

  • Your Name (unregistered) in reply to Naomi

    I thought I was the only one. Programmers scared of operator overloading? It's a practise that has its proper place and is well understood.

    If that scares you go back to java.

  • (nodebb)

    Everytime someone says "unit test" I cringe hard at our code base. I suspect that many un-designs would have been revised. if there WERE unit tests, because there is so much global state that needs to be set up in the correct order before you get reliable results... And since there are no unit tests, any little change has to be wrapped into very specific conditionals, in order not to break code that already works.

    You can imagine the readability.

  • Prime Mover (unregistered) in reply to R3D3

    First Java project I was on, back in the early Noughties, was still fairly young when the concept of unit testing was first floated. So we were shoe-horning JUnit into all our work. This had the unexpected side-effect of us making some subtle design changes in our application so as to be able to make possible the generation of meaningful, efficient and streamlined unit tests. This had the even more unexpected side-effect of improving the design of the overall application, and we found ourselves refactoring some less-than-high-quality code into something that was actually not all that bad. That was a lesson I took with me.

  • I dunno LOL ¯\(°_o)/¯ (unregistered)

    I'm pretty sure that "(a - b) > 0" is what you use when both values are strictly incrementing and can wrap around. So basically things like internal timestamps of milliseconds uptime, and TCP sequence numbers. You don't use it unless the numbers wrap around. And yeah, you want them to both be unsigned so that the result will be unsigned.

    I use a different arrangement of this for timers, roughly "if (now - t0) > delay". This avoids the "49th day failure" problem, when uptime reaches 4 billion milliseconds and the whole system goes nuts, or maybe half that if signed numbers are used. There are actual jet planes that have to be hard rebooted every few months because of timer wraparound bugs like this.

  • Alex (unregistered)

    I prefer to use x == null instead of x is null to be consistent with older versions of C# and comparing with a value that is not null. If using == causes a bug I would rather fix the overloaded operator rather than use is null. IMO the real purpose of is null is when you are dealing with a library that has a buggy overloaded operator that you can't fix.

  • (nodebb) in reply to Charles

    Not true, since signed integer overflow is already undefined behaviour, so the compiler can assume that (a - b) > 0 does not overflow (making it equivalent to a > b) and generate the optimal comparison code.

  • (nodebb)

    Wow, I haven't been to this site in a long time. And I haven't programmed in C# in even longer. I also have mostly not even followed it since 3.0. But I had to take a gander at the linked blog site.

    And I found this gem https://codeblog.jonskeet.uk/category/evilcode/

    First off, I think it's strange that a language has to create so many new (and confusing) operators to deal with null references. Is it just me, or does this actually seem like a symptom of a much larger problem?

    Anyway, nice to see the Daily WTF is still around and pumping out more nuttiness.

  • (nodebb) in reply to Steve_The_Cynic

    which works fine if both a and b are signed.

    No it doesn't. consider a = INT_MIN and b = 1.

Leave a comment on “WWJSD?”

Log In or post as a guest

Replying to comment #:

Log Out

« Return to Article