Comment On Code Ownership Gone Awry

So he took a string compare function and rewrote it to be a double compare function with a different name? I'm not sure I get it.

Excuse me while I rewrite this comment in ASM.

Why couldn't the original just have:
return !strcmp(left,right);

???

Why all the extra code?

Replying to myself... Or...
As I would have written it to be more readable
return strcmp(left, right) == 0;

so wait, is this a floating point compare or a string compare function?

... did he take floating point precision into account? is 1/10 == 1/20 + 1/20 using his compare? (or anything else that results in a repeating binary value past the radix point and SHOULD be equal)

also, I know some people like that... I work with them... one of them wrote a bad implementation of a 64-bit hexstring-to-longlongint converter function rather than using _strtoui64(). His code is a morass of cross-including messes, copy and paste replication, commenting of random chunks of code, horrible call trees that make a rube goldberg machine look reliable, and just plain WRONG code that blatantly flies in the face of the API documentation and all logic.

Ah our dear friend NIH syndrome is back. I wonder how many good articles we read only because it exists?

StringUtil_Compare(const char*, const char*)

...becomes...

Utilities_CompareDouble(double, double)

?

Both snippets made me say WTF

I would rewrite that function as well, tho maybe not using assembler:

inline bool StringUtil_Compare( const char * left, const char * right )

{

    return !strcmp( left, right );

}

Even more so, I'd probably avoid that function like the plague - i'd rather use strcmp directly.

"Despite the fact that he is a very competent, experienced programmer..."

I doub't it if he's wasting time 'optimising' code that doesn't need to be looked at. If it ain't broke don't fix it - it takes longer and could introduce more bugs. I used to do stuff like this when I was in university - but there was a purpose to that - it helped me learn how coding works.

STL sucks. It rightly should be called non-STL. You better test it well!

Somebody please check out this article, re-write it, and check it back in as entertaining.

Pim:

So he took a string compare function and rewrote it to be a double compare function with a different name? I'm not sure I get it.

I don't get it either. The functions do two totally different things.

- "if I didn't write it, it's not good enough"
- "This is the programmer who hates to use freely available libraries like STL, boost, or any third party API"

Some call this WTF, others call it the DJB style...

May actually may?

Maybe it's too early in the morning, but what's with the !strcmp part? It seems like it'd work the same way without the ! since if the strings are equal, strcmp returns 0 and !0 is still 0.

confused:

Maybe it's too early in the morning, but what's with the !strcmp part? It seems like it'd work the same way without the ! since if the strings are equal, strcmp returns 0 and !0 is still 0.

no

SatanClaus:

Pim:

So he took a string compare function and rewrote it to be a double compare function with a different name? I'm not sure I get it.

I don't get it either. The functions do two totally different things.

I believe they are two different examples, though the article doesn't make that clear.

Anyway, I wish I could read asm better. That would enable me to determine why he has ;23 for a comment. And why the function seems to return one of the values 12582913, 12582914 or 12582915.


Addendum (2009-03-20 09:39):

By the way, I could understand his assertion that the standard libraries aren't very efficient. If you know that your variables will never become NaN or Inf, why use a library that checks for those values each and every time you call it? Far better to roll your own; you might save a couple of nanoseconds during the program.

Archimidas:

I would rewrite that function as well, tho maybe not using assembler.

No you wouldn't. You'd remove it and replace all instances in the codebase with the call to the built-in comparison function. You would do this because you are a competent professional.

And then you would weather the hissy fit.

DemonWasp:

Archimidas:

I would rewrite that function as well, tho maybe not using assembler.

No you wouldn't. You'd remove it and replace all instances in the codebase with the call to the built-in comparison function. You would do this because you are a competent professional.

And then you would weather the hissy fit.

I'd rewrite it so I didn't have to search the codebase and replace them all - the inlined function should essentially take care of that for me. It can sometimes be easy to miss some (embedded in pre-processor macros, certain build configurations etc.).

THEN I'd throw a hissy and stop anyone using that damn function ever again.

I've seen typos and mix ups in WTF articles before, but posting two functions that do completely different things and saying one replaced the other sets a new standard.

confused:

Maybe it's too early in the morning, but what's with the !strcmp part? It seems like it'd work the same way without the ! since if the strings are equal, strcmp returns 0 and !0 is still 0.

!0 is 1

strcmp returns 0 when the strings match.
The function wants to return 1(true) if the strings match, so you can do
return !strcmp(left,right);

However, I find that ! in the above case is "confusing" because personally I only use ! for inverting a boolean type value. strcmp on the other hand returns either <0, 0 or >0 depending on the comparsion, so I find it clearer in this case to write
return strcmp(left,right) == 0;

confused:

Maybe it's too early in the morning, but what's with the !strcmp part? It seems like it'd work the same way without the ! since if the strings are equal, strcmp returns 0 and !0 is still 0.

It is indeed too early in the morning, for you, anyway. !0 is 1.

At least he just made a wrapper for the library function and didn't write the actual strcmp function himself.

No, it wouldn't. I can read asm just fine and I have the same questions. My first thought was that maybe the weird values are the integer representations of the floating-point values 1, 0, and -1, but 'tain't so. Presumably these values are those of an enum defined somewhere else. FILE_NOT_FOUND is probably 0x00c00004.

Of course any actually competent programmer would use std::string (or QString or whatever). Using char*'s is tedious and error prone.

TRWTF is that Jay thinks this guy is competent.

(his project is to be demoed soon)

"Demoed" took me a few moments to process. At first I thought, "They're going to remove one of the three stooges from his project?"

...hissy fits ensue complete with resultant silence treatment.

If the developer was female, this would be a hussy fit.


Side note: "silence treatment"?

The XML parser bit in the article I understand. There is a ton of XML parsers out there, many bad, and the acceptable ones force specific structures upon your code that may or may not fit.

I have yet to see a XML parser that is clean, fully standard-compliant and does not require you to overturn your data structures or code logic.

(Links welcome.)

So, that is the one part where I can understand why somebody might do that.

Steve the Cynic:

confused:

Maybe it's too early in the morning, but what's with the !strcmp part? It seems like it'd work the same way without the ! since if the strings are equal, strcmp returns 0 and !0 is still 0.

It is indeed too early in the morning, for you, anyway. !0 is 1.

And also, as it happens, 0! is 1.

I think my co-worker worded this odd. Maybe because he was giggling so hard at the time.

He didn't mean to say that the StringUtil_Compare function was the original version of the Utilities_CompareDouble, but that the Utilities_CompareDouble was the -original- version of that function. There was no 'simpiler' version, say done using if/else statements. The first and only version was all ASM.

Funny thing is, that code wrongs several massive orders of magnitude slower than a simple if/else statement.

Holy crap, and now I can't type either. LOL

The code is wrong, and it -runs- slower too!

CNinja:

There was no 'simpiler' version

simpiler: a. Used for building simulation programs.
b. A dumbed-down version of a compiler.

CNinja:

Funny thing is, that code wrongs several massive orders of magnitude slower than a simple if/else statement.

Dude, get some coffee. Like... now.

*EXACTLY* like my boss. do we refer to him here? the last gem is that he disallowed us to use strcoll() and we shall wait for his implementation. "official" reason being that he doesnt trust anyone that the strcoll definition will be up to date on every computer. so he writes his own version of the function along with an own set of databases that will be kept up to date...

The real WTF is that someone can explain all the crappy things this guy does and then still claim that this guy is a "very competent, experienced programmer".

NO. By definition, if someone is writing unreadable code that reinvents the wheel 100 times and is essentially unmaintainable, then that person is NOT a competent programmer. He's a hack that happens to know enough programming to make some stuff work.

We need to stop enabling people like this by calling them competent, or respecting them in any way. Realizing that you're part of a team and working with that team is more important than doing your own thing when developing.

There's nothing wrong with the double_compare code, except maybe a lack of comments.

It's doing a kind of compare you just can't do in C.

It not only does a 3-way compare, but the special FUCOMI instruction is the only compare instruction that does not raise errors if the numbers are undefined (NaN).

If you absolutely had to write this in C it would be along bit of code, something like:

if( IsNan( A ) || IsNan( B ) ) return 0xC0003;
else { Diff = A - B; if( Diff < 0.0 ) return 0xC0002;
else if( Diff > 0.0 ) return 0xC0001; else return 0xC0004;

I've done similar asm shortcuts, but only like in the inner loop of a sort routine that was called a bazillion times.



I don't understand the business with checking "ah", as FUCOMI does not modify ah. Maybe you left out a LAHF instruction that was in the original code?

NSCoder:

Steve the Cynic:

confused:

Maybe it's too early in the morning, but what's with the !strcmp part? It seems like it'd work the same way without the ! since if the strings are equal, strcmp returns 0 and !0 is still 0.

It is indeed too early in the morning, for you, anyway. !0 is 1.

And also, as it happens, 0! is 1.

Of course, !0 really ought to be FILE_NOT_FOUND

Just did actually, should have done that before posting. :)

Ok, Simpler. Although it is being used in Simulation, so does it still work?

Although I'm still trying to figure out how I got "wrongs" when I wanted "runs"...

Code Dependent:

"Demoed" took me a few moments to process. At first I thought, "They're going to remove one of the three stooges from his project?"

Perhaps being 'demoed' is exactly what the project needs. :-)

Inline ASM, the hacker's best friend and coder's worst nightmare. I wonder how many picoseconds he may have saved with his ASM solution?

!0 = 1

The double_compare is indeed OK, and using ASM for a 3-way-compare with FUCOMI makes perfect sense.

The WTF I see here is: what the hell is the connection between such a comparison of double values,
AND A STRING COMPARISON?

Maybe the OP left out some important parts - or the replacement made sense and he didn't get it.

I guess he put assembly language in there for job security reasons. It makes no sense to optimize some things, when all you do is create more bugs. You should do your best to write efficient code, but optimization should only be done in trouble areas. If he's going that far to make a change, why didn't he write the whole thing in assembly.

Anonymous:

Inline ASM, the hacker's best friend and coder's worst nightmare. I wonder how many picoseconds he may have saved with his ASM solution?

None, it runs slower.

Remember it's returning enumerated values... which he then has to do an if/else if/else test on, which adds even -more- branches to the prediction unit of the processor. If you just do the if/else if/else tests where they are needed instead of calling this function, it's much faster.

ASM isn't always faster.

TRWTF is that either of those functions exists at all.

Double comparison: Are > and < and = not good enough for you?

String comparison: Why in the heck would you wrap strcmp in a function that is notably stupider?

Steve the Cynic:

confused:

Maybe it's too early in the morning, but what's with the !strcmp part? It seems like it'd work the same way without the ! since if the strings are equal, strcmp returns 0 and !0 is still 0.

It is indeed too early in the morning, for you, anyway. !0 is 1.

!0 == 1
0! == 1
!0! == ??? ;)

Also I would have used !!!!!strcmp(str1,str2) just to show that it is important.

It's obvious that he doesn't like strings and has developed another precise algorithm (not shown here) to convert strings to floating point representations. Come on people!

Otto:

TRWTF is that either of those functions exists at all.

Double comparison: Are > and < and = not good enough for you?

As "Don't get it" pointed out, that asm comparison has two properties those operators do not:
It is a 3-way comparison instead of a boolean one,
and it can handle NaN values without throwing an exception.

If you need that, then the operators are not good enough indeed.

Can we get an RMA for this WTF?

CNinja:

Funny thing is, that code wrongs several massive orders of magnitude slower than a simple if/else statement.

Actually, that scanned perfectly fine. It sounds like something Calvin might say