-1

I want to have definition for pow2C as follows. But my compiler shows it (__tmpDouble__) is already defined. Please help me define it.

#ifndef pow2C
double __tmpDouble__;
#define pow2C(a) ((__tmpDouble__=(a))*(__tmpDouble__))
#endif

.
.
.

inline double calculateDistance(double *x, double *y, int NDims) 
{
    double d = 0;
    for (int i = 0; i < NDims; i++)
    // it is faster to calculate the difference once
       d += pow2C(x[i] - y[i]);
    return sqrt(d);
}
remo
  • 880
  • 2
  • 14
  • 32
  • 1
    So rename the variable to something else. – OldProgrammer Dec 20 '13 at 14:41
  • 3
    Names with double underscores are reserved. – leemes Dec 20 '13 at 14:42
  • 1
    it is __never__ a good idea to name something beginning with two underscores anyway. check the standard, such names are reserved – codeling Dec 20 '13 at 14:42
  • I want to calculate x[i]-y[i] only once and save it in a variable and use it. What do you mean of rename? – remo Dec 20 '13 at 14:42
  • 1
    The question title doesn't describe the problem. Not really. – Lightness Races in Orbit Dec 20 '13 at 14:42
  • Please suggest your title! I will rename it. – remo Dec 20 '13 at 14:43
  • @RezaMortazavi Why do you want to use a macro for this in 1st place?? – πάντα ῥεῖ Dec 20 '13 at 14:44
  • 6
    The solution: don’t use a macro, use a function. – Konrad Rudolph Dec 20 '13 at 14:46
  • Double underscores are reserved, and you shouldn't use a macro for this anyway. – derpface Dec 20 '13 at 14:46
  • 1
    In all probability, the minimal performance enhancement you would achieve by saving one subtraction would be eaten up by hitting that global. A much better optimisation would be to work with square distances when possible, in order to avoid the square root. – molbdnilo Dec 20 '13 at 14:52
  • 1
    I'm reasonably certain that an expression of the form `(x=foo)*x` invokes the undefined behavior concept... If you just do `[static] inline double pow2C(double x) { return x*x; }`, you'll only evaluate the parameter once, before the "call" is made... And it's a lot more type-safe and will prevent odd behavior from calling it with complex expressions as argument... – twalberg Dec 20 '13 at 15:01

3 Answers3

8

If you insist on writing a macro, use a different name for the variable. But your macro is very terrible. First, it isn't thread safe with regard to concurrent calls, since the threads would access the same temporary variable. Also, most macros are very evil in general (there are some exceptions though).

However, the same (also performance-wise) can be achieved much easier (even with support for float and other types with operator* with a function template:

template <typename T>
T square(const T &v) {
    return v * v;
}

If you want a macro, then #define pow2C square (sarcasm sign).

leemes
  • 44,967
  • 21
  • 135
  • 183
5

That's a terrible way to define a macro! You should never introduce variables as part of the calculation. Instead, try this:

#define pow2C(a) ((a) * (a))

However, macros are terrible for this sort of computation as they can be inefficient and cause unwanted side effects. For example, if you code it would expand to:

 (x[i] - y[i]) * (x[i] - y[i])

Where you've done the subtraction twice!

Instead, use C++ templates:

template<class T>
T pow2C(const T &value)
{
  return value * value;
}

Now you'll only do the subtraction calculation once!

Sean
  • 60,939
  • 11
  • 97
  • 136
1

I want to calculate x[i]-y[i] only once and save it in a variable and use it.

You already do only calculate it once. This is all pointless.

If you still insist, then the error you name does not exist in this code.

However, you do have a problem with the content of the macro, which both reads from and writes to a single variable within an expression. The result is undefined:

#include <iostream>
#include <cmath>
#include <array>

#ifndef pow2C
double __tmpDouble__;
#define pow2C(a) ((__tmpDouble__=(a))*(__tmpDouble__))
#endif


inline double calculateDistance(double *x, double *y, int NDims) 
{
    double d = 0;
    for (int i = 0; i < NDims; i++)
    // it is faster to calculate the difference once
       d += pow2C(x[i] - y[i]);
    return sqrt(d);
}

int main()
{
    const size_t NUM_DIMS = 3;
    std::array<double, NUM_DIMS> x{{5.0, 6.0, 7.0}};
    std::array<double, NUM_DIMS> y{{1.2, 1.5, 9.0}};

    std::cout << "Distance between x and y is: " << calculateDistance(&x[0], &y[0], NUM_DIMS) << '\n';
}

// g++-4.8 -std=c++11 -Wall -pedantic -pthread main.cpp && ./a.out
// main.cpp: In function 'double calculateDistance(double*, double*, int)':
// main.cpp:16:31: warning: operation on '__tmpDouble__' may be undefined [-Wsequence-point]
//         d += pow2C(x[i] - y[i]);
//                                ^
// Distance between x and y is: 6.22013

So, really, we're back to don't do this.


If you're still desperate to avoid evaluating x[i] - y[i] twice:

inline double calculateDistance(double* x, double* y, int NDims) 
{
    double d = 0;
    for (int i = 0; i < NDims; i++) {
       const double diff = x[i] - y[i];
       d += diff * diff;
    }

    return sqrt(d);
}

You can pass off the multiplication work to another inline utility function if you like, but there is no need for a macro here.

Lightness Races in Orbit
  • 378,754
  • 76
  • 643
  • 1,055
  • If I use #define pow2(a) ((a)*(a)), when 'a' is complex, the compiler will calculate it twice. As my experiments confirm, this approach pow2C is more faster. but the problem of redefinition exists! – remo Dec 20 '13 at 14:45
  • 1
    “You already do only calculate it once.” – Not with a naive macro. – Konrad Rudolph Dec 20 '13 at 14:45
  • He tries to avoid the problem of the macro `#define pow2(a) ((a)*(a))` which evaluates `a` twice. I guess his statement only underlines why he uses such a solution. – leemes Dec 20 '13 at 14:46
  • @Reza: What is so "complex" about `x[i] - y[i]` with type `double`? This is ludicrous. – Lightness Races in Orbit Dec 20 '13 at 14:46
  • The error probably *does* exist. First off, since the name starts with double underscore, anything goes in Evil++. More probably OP is using this code in a header which is included in several TUs. – Konrad Rudolph Dec 20 '13 at 14:48
  • @KonradRudolph: The point I was trying to make, perhaps too subtly, is that the OP failed to _demonstrate_ a testcase that reproduces that issue. But it's all moot - read the rest of my answer and you'll see worse issues. – Lightness Races in Orbit Dec 20 '13 at 14:50