• snoofle (unregistered)

    "it’s been running this way for over 20 years"

    So have most of the cow-orker's I've had to deal with, but they keep going too!

  • Tinkle (unregistered)

    Got to love the buffer underflow, that should have caused random bugs in other parts of the application. Full WTF points there.

    element[-1] gets written to when removing non existing elements from the list.

    Also, is the order of the list important? If not simply copy the last item in the list into the vacated spot.

  • bvs23bkv33 (unregistered)

    element[i] = element[++i];

  • Ted (unregistered)

    Unnecessary while loop is unnecessary.

    if i == num - 1, element[i+1] will be a non-existent item

    that DelIndex exists and attempts to check for numbers to large but not too small (if it exists it must be called elsewhere, so we're not only dealing with the remove implementation here)

    and of course, the fact that their index checking doesn't work or do anything anyway which is the premise of the article.

    I hate this code. Thanks for reminding me that it could always be worse than my current project TheDailyWtf

  • RLB (unregistered) in reply to bvs23bkv33

    element[i] = element[++i];

    Elementary undefined behaviour, of the kind I only expect from C++ or C# programmers. Enjoy your nasal demons.

  • RLB (unregistered) in reply to Tinkle

    Also, is the order of the list important? If not simply copy the last item in the list into the vacated spot.

    It's used for GUI elements - I expect order is important in at least some cases. (Don't you hate it when the tab order changes? I do.)

  • Will K (unregistered)

    Isn't there another bug in DelIndex? In the last iteration, i == num - 1, so it's doing element[num - 1] = element[num]. Unless element is allocated to have an extra entry, this looks like an out of bounds access.

  • NoLand (unregistered) in reply to Will K

    "num" has already been decremented before, so the upper loop boundary is "num-1". Should be fine as long as 0 >= i <= num.

  • Ruts (unregistered)

    this looks like an out of bounds access. That's the great thing about C/C++, there is no 'out of bounds' limitation, it is all just pointer arithmetic. Sometimes there are no bounds ;-)

  • Ruts (unregistered)

    Oops, formatting screwed me there. Meant to say

    Re: this looks like an out of bounds access.

    That's the great thing about C/C++, there is no 'out of bounds' limitation, it is all just pointer arithmetic. Sometimes there are no bounds ;-)

  • Decius (unregistered)

    If the aim command of 'Ready, aim, fire' fails, the fire command should not be executed.

  • A Mommy Noose (unregistered)
    "We have no idea how this ever worked, or what might be affected by fixing it, but it’s been running this way for over 20 years."

    Jesus, this hurts me so much... this is my day, every day. Fix an obvious bug in one part of the code that has no obvious connection to breaking behavior in a completely different part of the code base that DEPENDED on that aberrant behavior.

  • Someone (unregistered)

    It's not out of bounds because of the num--, which might suggest a memory leak. Also, I guess it worked because they used to actually test things in debug mode, check the logs and fix the calls with non-existing entries.

  • ooOOooGa (unregistered)

    Bonus points for not actually deleting anything in DelIndex.

    And an even bigger WTF than the memory leak would be to define the array access operator or the assignment operator to free memory as a side effect.

  • sizer99 (google)

    I see this a lot, and it always tweaks me. It's completely lazy programmer.

    • Better check that x != null
    • Ugh, error handing is haaaaard, eff it, let's just put an assert in.
    • Damn, this code is constantly asserting.
    • Let's make it a log
    • Geeze, this is noisy
    • Let's turn off the log
    • Now you've got 'if( x == null ) /* do nothing */; everywhere. No better than doing no error checking at all.
  • xtal256 (unregistered) in reply to Decius

    That's how you blow your foot off.

  • Olivier (unregistered)

    "If you try and remove an item which isn’t in the list, that’s exactly what happens."

    If t is not one of the element[i], the for loops until i is zero and the first element is removed. That may not be the expected behaviour, but that should not cause some memory error.

  • (nodebb) in reply to Olivier

    If there is no match with element[0] then i gets decremented once more.

  • Abhijit Parida (github)

    In today's news: LoudAssert is surprisingly not loud!

  • Nick (unregistered)

    I would be willing to be that this worked perfectly well for the first 10-15 years of it’s life... maybe longer.

    Originally, the developers who wrote it knew exactly what was in the List, and only removed things that it definitely contained. If they weren’t sure, they wrapped the call with a check to see if the list contained the entry first... “if (list contains X) { list.remove(X); }” or equivalent.

    However, recently, new developers joined, and expected that the home-built list implementation worked just like the standard ones in many other languages... so assumed that “list.remove(X);” was a no-op when X wasn’t in the list...

    The bugs have then arrived either in new code built this way, or where new developers have tidied up and streamlined codes, and removed what they thought were redundant checks...

    The nice thing about this theory? No-one has actually done anything that stupid... The original developers built a list implementation that worked for them... The new developers made a sensible assumption... The only real mistake was not refactoring to use a standard library when one became available, but that’s generally a sensible move anyway in a large codebase.

    Amazing how many small, correct decisions can lead to one giant WTF!

  • Obfuscator (unregistered)

    I like to go with Mel:

    "If a program can't rewrite its own code what good is it?"

    So this is a very good program.

    Just in case you don't know Mel. https://www.cs.utah.edu/~elb/folklore/mel.html

    Bye

  • (nodebb)

    Not the first time I've heard the theory that the best thing to do with errors is ignore them. At a shop I once worked, we lost a whole day's user input batch because the output database got full part way through the run and errors were ignored.

  • linepro (unregistered) in reply to RLB

    Get it right the classic mistake is:

    *element = *element+1;

  • WTFGuy (unregistered) in reply to Nick

    @Nick: THIS. EXACTLY.

    Back in the day the design assumption was that everybody was an expert at their local internal APIs & homebrew libraries. So these things were often not built defensively. GIGO was the attitude: if you pass junk into a library call, you deserve the nasal d[a]emons that come back out.

    In the last 20ish years, with the explosion in copy-paste-from-stackoverflow as a mainstream development process, and the huge dilution in dev quality due to the huge explosion in dev quantity, any 21st century library should be very defensively designed and coded.

    As well, a more defensive library design simplifies use of the "fluent" style where a one-liner 10 method chain with a couple of lambdas in the middle is considered clear and concise coding.

  • Chris (unregistered)

    Interesting that, by design, it removes the last item in the list that matches the parameter, if there are multiple matching items. Probably never used on lists that have multiple copies of any items, but it is a list, not a set, and such behaviour may not be expected.

  • RLB (unregistered) in reply to linepro

    Get it right the classic mistake is:

    *element = *element+1;

    No, because that doesn't invoke nasal demons. bvs23bkv33's line does.

  • giammin (unregistered) in reply to RLB

    I am a c# developer but i dont have demons running outside of my nose

  • learning_cpp (unregistered)

    If i is -1 in DelIndex won't this: element[i] break due to segmentation fault?

  • medievalist (unregistered)

    "In our system stderr is silenced, always"

    Get out as quickly as possible. Run, don't walk. Don't look back.

Leave a comment on “Remove This”

Log In or post as a guest

Replying to comment #:

Log Out

« Return to Article