53

I wrote a function recently (with help of one guy from SO) that finds maximum of two ints. Here is the code:

long get_max (long(*a)(long(*)(long(*)()), long(*)(long(*)(long * *))), long(* b)
  (long(*)(long(*)()),long*,long(*)(long(*)()))){return (long)((((long(*)(long(*)
  (long(*)()),long(*)(long(*)())))a)>((long(*)(long(*)(long(*)()),long(*)(long(*)
  ())))b))?((long(*)(long(*)(long(*)()), long(*)(long(*)())))a):((long(*)(long(*)
  (long(*)()),long(*)(long(*)())))b));} /*p*/ /*a*/ /*d*/ /*d*/ /*i*/ /*n*/ /*g*/

#include <iostream>

int main()
{
  long x = get_max(
    (long(*)(long(*)(long(*)()),long(*)(long(*)(long**)))) 500,
    (long(*)(long(*)(long(*)()),long*,long(*)(long(*)()))) 100 );

  std::cout << x << std::endl; // print 500 as expected
  return 0;
}

It works fine, but my colleague says that I shouldn't use C style casts. But I think that all that modern static_cast's and reinterpret_cast's will make my code too cumbersome. Who's right? Should I redesign my code using C++ style casts or is original code OK?

49

Nah. This code looks fine to me.

I would make the following suggestions, though:

The ternary operator is meant to be used three times. That's why it's called the ternary operator. For example:

Wrong:

return a>b ? a : b;

Right:

return a>b?(a>b?a:b):(a<b?b:a);

Note that this rule is recursive:

((a>b?(a>b?a:b):(a<b?b:a))>(a>b?(a>b?a:b):(a<b?b:a))?((a>b?(a>b?a:b):(a<b?b:a))>(a>b?(a>b?a:b):(a<b?b:a))?(a>b?(a>b?a:b):(a<b?b:a)):(a>b?(a>b?a:b):(a<b?b:a))):((a>b?(a>b?a:b):(a<b?b:a))<(a>b?(a>b?a:b):(a<b?b:a))?(a>b?(a>b?a:b):(a<b?b:a)):(a>b?(a>b?a:b):(a<b?b:a))))

This code may be correct, but it probably isn't. I find that the best way to debug it is to put it in a nice, critical function in some production code. Inevitably, it gets fixed if there's anything wrong with it.

Make sure you don't introduce unnecessary line breaks or white space, since they slow down the preprocessor.

14 accepted

You cannot use static_cast (5.2.9), but it is allowed by reinterpret_cast (5.2.10/5). A C-style cast (5.4/4) can try to perform a const_cast, static_cast, const_cast+static_cast, reinterpret_cast or const_cast+reinterpret_cast in that order, which is also valid. With modern cast one needs to spend precious time fighting with the compiler to decide which of the 4 is allowed be used, taking exponentially longer time (O(4n)) to debug as the system scales up, and reducing productivity. But a C style cast is a quick way to resolve the problem without hassle. So you could just explain the benefits and throw the C++ standard to your colleague saying this casting is approved by the committee.

But if you're paid by character count, listen to him/her and switch to reinterpret_cast<long(*)(long(*)(long(*)()),long(*)(long(*)(long**)))>.


And I don't think there's anything wrong to declare a function of pointer to function of pointer to function of pointer to function returning int returning int and pointer to function of pointer to function of pointer to pointer to int returning int returning int returning int named as a, and pointer to function of pointer to function of pointer to function returning int returning int, pointer to int and pointer to function of pointer to function returning int returning int returning int named as b, returning int.

Right?

(Ref: http://cdecl.org/)

13

The original code is OK

11

You should use jQuery to call the Math.Max function instead of rolling your own.

5

No. Your colleagues clearly do not know what they are talking about.

That code is a thing of beauty; I'm only sorry I did not write it myself.

5

No, definitely no need to redesign.

I find your code totally working and very readable. It should be an excellent example in our coding standard document.

5

I'd change the output line as such:

 std::cout << 500 << std::endl; // print 500 as expected

Since you want to print 500 anyway.

4

Only if he's suggesting you use templates... I want to use longs!!

4

The code looks fine for this use-case, but in general you might want to make it more flexible to allow for easier reuse:

template<typename A, typename B, template<typename> class C, template<typename> class 
  D, template<typename, typename> class E> typename E<typename C<A>::type, typename 
  D<B>::type>::type get_max(A a, B b) { return E<typename C<A>::type, typename 
  D<B>::type>::op(C<A>::get(a), D<B>::get(b)); }
2

You may want to convert the ints to longs to avoid overflow errors during the comparison.

1

The code itself is perfect, but some formatting and whitespace might clear up your coworker's confusion.

0

Wait, I can't get it to compile. g++ says:

main.cpp:13: error: "cout" was not declared in this scope
main.cpp:13: error: "endl" was not declared in this scope

What'd I do wrong?

0

You really should generalise your max function. Computing the max of two objects is really like finding the supremum that is attained by a convergent sequence (of objects).

Right now it is too specific to be of general use.

-1

Your current version of the code takes in two longs, implicitly casting the ints the user passes in to long. This does not match your description of the function, which claims it takes in two ints. You should have the function take in ints and explicitly cast them to long. As is, users may incorrectly pass in longs to your function, adding back all those conversion errors oded mentioned, which were the motiviation for all the long casting in the first place.

-2

This is a troll, right?

In the real world, other people have to be able to read and understand your code. Since there's at least one standard library function that solves this problem already, namely std::max from < algorithm >, not using it is simply wrong. Wherever there is a standard solution to a problem, using anything else is by definition harder to read and understand, and therefore strictly worse.

Your colleague is right to criticize this code, although the C-style casts are certainly not the only, or even the first reason why.

To answer your specific question, casts mask errors. The C++-style casts are "safer" in the sense that the compiler is able and required to diagnose more erroneous usages of them than it can and does for C-style casts.

For example:

int main(void)
{
   void (*fp)(void) = (void(*)(void))(5);  // utter nonsense
   (*fp)();
}

This rubbish is happily compiled without diagnostics by g++:

janks@phoenix:/tmp$ g++ -Wall -ansi -pedantic nonsense.cc && ./a.out
Segmentation fault

But most of the C++-style casts are required to be diagnosed by the compiler when using them for a nonsense conversion like this:

int main(void)
{
   void (*fp)(void) = (void(*)(void))(5);
   fp = static_cast<void(*)(void)>(5);
   fp = dynamic_cast<void(*)(void)>(5);
   fp = const_cast<void(*)(void)>(5);
   fp = reinterpret_cast<void(*)(void)>(5);
   (*fp)();
}

janks@phoenix:/tmp$ g++ -Wall -ansi -pedantic nonsense2.cc 
nonsense2.cc: In function 'int main()':
nonsense2.cc:5: error: invalid static_cast from type 'int' to type 'void (*)()'
nonsense2.cc:6: error: cannot dynamic_cast '5' (of type 'int') to type 'void (*)()' (target is not pointer or reference to class)
nonsense2.cc:7: error: invalid use of const_cast with type 'void (*)()', which is a pointer or reference to a function type

Only the reinterpret_cast "works" here, in the sense that it will compile. (Obviously it will core in a hurry when you try to run it.)

So you should prefer C++-style casts wherever possible. Fewer errors are masked by them than by their C-style equivalent. reinterpret_cast<> is the most permissive of the casts, so it elicits the fewest diagnostics. But you shouldn't be using it unless you know what you're doing. For what you were trying to do, you'd use a static_cast<>, and the compiler would stop you if the cast made no sense.