• Derp (unregistered)

    Posting here without declaring frist to annoy fristposters

  • Foo AKA Fooo (unregistered)

    You can write crap in any language, or using any language feature. The same monstrosity could be written as a getFoo() function, so do you want to ban functions named "get..."? Operator overloading can be useful in many cases, and the only thing that helps against its misuse is the same as always, code review.

  • Nobody (unregistered)

    The idea of lazily evaluating "how long to go before the timeout" is not wrong in itself. The only wrong thing there is that the class name needs to be something like "ClockTicksRemaining" so that the DWORD operator makes sense as returning that value. Timers don't have a numeric value, so if the class name isn't changed, a clockTicksRemaining member function needs to replace the DWORD operator.

    You are wrong about no bugs, though. Admittedly I've only read the code three times, but it seems to me that if we are past the expiry time, the result will be a negative value cast to a DWORD, and hence definitely not zero. This is part of the other bug in the design, which is that a DWORD value "clock ticks remaining" is being silently cast to a 'bool' value "clock not completed yet".

  • Zeh (unregistered)

    It's not coded wrong but its wrongly coded.

  • (nodebb)

    I'm a tiny bit disappointed. When I run into magic code like this I always expect to see a new WhiteRabbit(); Not here though :(.

  • isthisunique (unregistered) 
        if (dwCurrentTick < m_dwStartTicks)
        dwElapsed = (UINT_MAX - m_dwStartTicks) + dwCurrentTick;

    Ouch.

  • Richard (unregistered) in reply to Foo AKA Fooo

    The same monstrosity could be written as a getFoo() function, so do you want to ban functions named "get..."? That's quite an extrapolation, and I'm not sure how you get to that conclusion :) The monstrosity here (according to the final line of the article) is the decision to implement using an operator, not the contents of the methods. If this was written as a getFoo() method, at least the reader of the source code would be able to see that something was being done. But by putting that functionality in a cast operator the call to that "method" is completely hidden - that's the monstrosity.

  • just me (unregistered)

    People how can't get this kind of operator overloading should definitely stay away from C++ (and python, please, too)... I mean sorry, the class is called "CTimerElapsed". A timer is the very essence of something which changes by itself, so if you go all "OMG this changes by itself" I'd have to answer "So, umm, what exactly did you expect a timer to do?"

    Seriously, operator overloading is an essential language feature. A language must allow custom types which can be used like built-in types, and operator overloading is essential for this, otherwise code written in it quickly becomes unreadable due to the unnecessarily heavy syntax (what's more readable: "x.DivideBy(a.Plus(b.Minus(1)))" or"x / (a + (b-1))"?) and arbitrary syntactic differences depending on the types you happen to operate on.

    Yes, custom operators can be abused (and notwithstanding the above I can accept that this is a bit of a limit case). That's just bad function naming, nothing more, nothing less. Forbidding operator overloading does not prevent bad function naming. If you can't trust your co-workers to use operator overloading correctly, you can't trust them to correctly name any of their functions.

  • (nodebb) in reply to Nobody

    (gets pendant on)

    Actually these are milliseconds, not ticks, so the class should be named MillisecondsRemaining.

    (takes off pendant)

  • just me (unregistered) in reply to Richard

    The monstrosity here (according to the final line of the article) is the decision to implement using an operator

    Because you would expect casting an object of some type called CTimerElapsed to a DWORD to result in what, exactly?

  • just me (unregistered)

    For the record: I absolutely don't think this was good design, but for much more specific reasons than "I can't get used to operator overloading". Providing an implicit conversion operator to an integer type is (almost) never a good idea in C++, because it will cause function resolution ambiguities in the weirdest places and get called where you least expect it (such as in that while() condition there), and this is definitely a C++ (language) WTF.

  • Appalled (unregistered)

    Many languages allow differing levels of obfuscation. I don't know C++ at all but I'm guessing this is Extreme Obfuscation as opposed to Simple Obfuscation (in C# for example, coding database IO into Gets and Sets).
    Thank God I'm at a primitive facility where 80% of the work is simple PHP, with NO CLASSES or OBJECTS, where no levels of obfuscation other than Requires and Includes are available. And while we're at it, obfuscation is NOT new. The remaining 20% is 10 year old VB/VBA/C# and yes, the predecessors used every imaginable Class and quasi-Class manner of obfuscation they could find. Truly sucks, and truly disappearing as fast as I need to work on the crap.

  • Michael (unregistered)

    Clearly the bug is a simple typo. The function passes in dwTieout, but tries to use dwTimeout. dwTimeout is not declared in this scope...

  • fristy (unregistered)

    Medinoc am I missing something or are you confusing pedantic with pendants?

  • just me (unregistered) in reply to fristy

    Medinoc am I missing something or are you confusing pedantic with pendants?

    Both.

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

    Anyone who uses parens in a return statement in C/C++ (as though it was a function) has already demonstrated a fundamental lack of understanding of the basic principles of the language. The rest of the WTFery follows.

  • Paul Neumann (unregistered) in reply to fristy

    @fristy, Quit being so pendantic!

  • DCRoss (unregistered)

    This code is not wrong, Walter. It's just an asshole.

  • (nodebb) in reply to fristy

    Have you visited the forums? The pendant emoticon is a symbol for pedantry, just like the trolleybus emoticon symbolizes trolling.

  • isthisunique (unregistered) in reply to Appalled

    It seems like obfuscation through being too terse. It's a common problem in highly dynamic or configurable languages.

  • Herby (unregistered)

    autopodóplo - I like it. Probably done it a few times myself, but I've seen many instances of such behavior.

    Funny it seems to always be in "versatile" languages that "everybody" says are "good".

    Sometimes I feel that going back to Fortran-66 is a good exercise, just to keep me grounded. No fluff there!!

  • Andrew (unregistered) in reply to Foo AKA Fooo

    Absolutely Foo AKA Fooo: code reviews are a must. I've seen plenty of perversions of C#'s set {} get {} accessors of properties which do (essentially) the same thing: manipulating many other things beyond just private data accessed by the accessor. I rather like WTFs like this one. It allows a healthy chuckle and nice bit of reference the next time I'm tempted to try something "cool" which may work but causes other types of issues: especially with maintenance (or debugging).

  • Loren Pechtel (google)

    I have to agree with just me, this is just a case of very bad naming and relying on a DWORD -> bool implicit cast. The fact that it's overloading isn't all that apparent or important, imagine if CTimerElapsed wasn't a DWORD but a proper class. You would have to allocate the instance and you would have to refer to some getter instead of just the class.

    If I saw a timeout that didn't get changed but was a class (as this appears to be since it's declared to be a CTimerElapsed) I would figure it was being done behind the scenes.

  • DWalker07 (unregistered) in reply to Medinoc

    Pendant? Hmm......

  • Chris (unregistered)

    This is C++, not C. I thought it meant to be highly optimised for blowing your whole leg off, not just your foot?

  • (nodebb) in reply to Nobody

    @Nobody:

    Admittedly I've only read the code three times, but it seems to me that if we are past the expiry time, the result will be a negative value cast to a DWORD, and hence definitely not zero.

    I don't think so. If dwElapsed is greater than m_dwTimeout, wait is never changed from its initial value of 0, so the cast will return 0.

    I don't think the article describes it very well though. There's not some hidden counter that's only being decremented when we do a cast to DWORD. Rather, the DWORD cast tells you how many ticks are remaining, if any. That actually seems pretty reasonable to me.

    Addendum 2017-01-30 21:13: Granted, it'd probably be better to put this logic in a property getter called something like TicksRemaining.

  • Foo AKA Fooo (unregistered) in reply to Chris

    In C++, the least you should do is write a template to shoot any body part of any person (or duck).

  • Anonymous (unregistered)

    What is the point of this while loop and the timer class here?

    while (MilliSecondsRemaining -= some_value > 0) { // timeout value passed into the function is decremented WaitForSingleObject(EventHandle, MilliSecondsRemaining -= some_value); // and is again decremented before function call ... } This means WaitForSingleObject() will wait for a shorter than passed in timeout first time, and then wait interval will decrease with each while loop pass unless it either succeeds or fails.

    If (reason == dwEvent && value == nRecord) does not evaluate to true but the event handle keeps getting signalled, the timeout will be much, much longer than the timeout value passed into the function.

  • Geek (unregistered) in reply to Foo AKA Fooo

    @Foo AKA Foo - why should Chris duck? He might not want to write a template, but that doesn't mean he should duck.

    I've worked with a person (I hesitate to call him a coder) who loved to overload type conversion operators to do weird things. I believe a type conversion should be idempotent and free from side-effects - this one is not. Thank you for bringing back such painful memories - how about you give me a paper cut and pour lemon juice on it?

  • (nodebb) in reply to isthisunique

    Indeed, thanks to the magic of C++ unsigned arithmetic, this is just the same as dwElapsed = dwCurrentTick - m_dwStartTicks - 1; - what they probably wanted was dwElapsed = dwCurrentTick - m_dwStartTicks; which conveniently makes the if/else redundant.

  • Sabre (unregistered)

    Perhaps autopodipyrobolo for "shooting" oneself in the foot?

    Disclaimer: I, too, am a programmer, not a linguist.

  • Klimax (unregistered)

    First offense is overloading wrong operator. It should overload decrement operator so what's going on in original loop is much clearer.

    Anyway, properly used overload of operators leads to better code. This doesn't conform to that...

  • progger (unregistered) in reply to Foo AKA Fooo

    Leave the ducks out of it! :quack:

  • Ron Fox (google) in reply to Medinoc

    Pedant?

  • nasch (unregistered) in reply to Appalled

    Maybe there is sarcasm going on here but it doesn't look like it. I got a kick out of someone expressing relief at working in PHP so they don't have all the WTFness.

  • (nodebb) in reply to isthisunique

    [quote=ukerab] Indeed, thanks to the magic of C++ unsigned arithmetic, this is just the same as dwElapsed = dwCurrentTick - m_dwStartTicks - 1; - what they probably wanted was dwElapsed = dwCurrentTick - m_dwStartTicks; which conveniently makes the if/else redundant. [/quote]

    Nice catch. The computation was off by one tick in the case of overflow. Meaning on a bad day, there's a real slight chance the timeout loop would go one more iteration than it needed to.

    But for the off-by-1, which really has no operational cost, the CTimerElapsed class seems to be custom made to feed the second argument to WaitForSingleObject (or one of its brethren). Maybe a silly technique, but definitely intended.

    I find a lot more questions reading the main code:

    • Race condition in the time interval between WaitForSingleObject and Signal.Pop()? Might be OK, depending upon implementation of CMySignal, which we don't know.
    • Loop keeps eating Signals until it gets the one it wants? Does the system generate a bunch of signals that no one else cares about? It might be better to check with CMySignal::Peek() first, but then if the signal you want is buried, and nobody else is processing them, you might loop the full timeout interval.
    • Race condition where (DWORD) timeout is not zero at end of loop, so the loop continues, but is zero by the time it hits the call to WaitForSingleObject() in the next iteration. There seem to be reports of some singular behavior when the dwTimeout is 0, although the worst consequence seems to be "I missed a signal that I might have caught, right before I was going to give up anyway."
  • Appropriate Analogy (unregistered)

    "this is just a case of very bad naming and relying on a DWORD -> bool implicit cast."

    This is like saying, "he didn't rob a store, he just demanded the contents of the register while holding the clerk at gunpoint."

  • Ilan Keshet (github)

    'autopodóplo' is a really awesome word, The only problem is that, using Google, I can't find any references to it other than this page.

    do you have any concrete source where this term is used prior to here?

  • Gustav (unregistered)

    So let me get this straight, timeout is decreased when it is used in WaitForSingleObject() where it is implicitly converted to a DWORD, but not in while(timeout) where it is implicitly converted to a bool? Or is the cast to bool implicitly using the cast to DWORD too?

    Brilliant use of overloading with hidden side effects...

  • Damon (unregistered)

    Anyone notice this? Typo in the code wouldn't compile.. just a type in the article?

    bool CMyDatastore::NetLockWait(DWORD dwEvent, long nRecord, CMySignal& Signal, DWORD dwTieout)
{
    timeout.SetTime(dwTimeout);
}

    Tieout vs Timeout

  • Mark Tarrabain (google)

    Fundamentally, using operator overloading for purposes that are not readily understood by readers without having to read anything less than a short essay justifying why that operator was even chosen to do that is not any different than using function names that are also meaningless to the reader, such as asghr() or koopr().

Leave a comment on “Overloaded Loop”

Log In or post as a guest

Replying to comment #:

Log Out

« Return to Article