11
\$\begingroup\$

I'm using this little macro a lot:

#define RUN_ONCE(runcode) \
{ \
    static bool code_ran = 0; \
    if(!code_ran){ \ 
        code_ran = 1; \
        runcode; \
    } \
}

I find it useful when I want to initialize stuff just once when I don't really care about performance (for example, if it's inside a render loop, it takes just 60 if's per sec). In a random generator, I can do RUN_ONCE(init_random()), so I don't need to separately initialize it, which prevents me from crashing when I call the function without initializing it first.

I'd like to know if any of you use a similar system, and if there's better way of doing this. Or if I should stop using this method immediately...

\$\endgroup\$
  • 2
    \$\begingroup\$ Note that since C++11 magic statics guarantees that a static value in function scope will be initialised exactly once even in the presence of multiple threads. static int result_of_run_once = runcode() will do it right if your runcode() has a return value. Otherwise use std::call_once also since C++11. \$\endgroup\$ – Emily L. Dec 21 '16 at 15:53
16
\$\begingroup\$

It is not thread safe. You can use Boost, or standard C++11 std::call_once.

Answer to comment:

Boost and C++11 are defining include library for launching threads and thread synchronization (locks, atomic variables…). The call_once function can either use those to ensure thread safety or use the thread lib of the OS (pthreads for *nix).

A very simplified (unefficent) implementation might be:

static std::mutex mutex;
static bool called = false;
{
   std::lock_guard<std::mutex> lock(mutex);
   if (!called) {
      f(); // <- User code
      called = true;
   }
}

It is not very efficient because the lock is always taken. An optimisation it to atomically check the once flag before taking the mutex:

static std::mutex mutex;
static std::atomic<bool> called = false;
{
   if (!called) {
     std::lock_guard<std::mutex> lock(mutex);
     if (!called) {
        f(); // <- User code
        called = true;
     }
   }
}

The implementation of pthread_once in glibc is interesting: it is much more complicated as it tries to behave correctly in presence of fork.

Boost/C++11 use a functor (function pointer or object with operator()): this way it can be implemented as a function and not a macro (you can use lambda in C++11 to avoid defining a separate function).

\$\endgroup\$
  • 1
    \$\begingroup\$ just out of interest, how does boost solve this problem about thread safety? also, the boost needs a function pointer, why is that better? i wouldnt want to make a function for every of these things, thats the whole point of my method... \$\endgroup\$ – CocoCoder Aug 27 '11 at 0:50
  • \$\begingroup\$ The code is more complicated because it deals with multiple threads concurrently entering the function. If so, only one thread should run the code, but all should wait until it is done. \$\endgroup\$ – Sebastian Redl Mar 12 '14 at 11:04
  • \$\begingroup\$ @SebastianRedl, it is already handled by the mutex. \$\endgroup\$ – ysdx Sep 24 '15 at 11:07
  • 1
    \$\begingroup\$ @ysdx with std::atomic<> one can use thefetch_or() method to simplify the code a lot. I show how to do it here: codereview.stackexchange.com/a/49168/42067 \$\endgroup\$ – Fabio A. Dec 21 '16 at 17:25
12
\$\begingroup\$

Note that macros such as this should be placed in a do { ... } while (0) block, otherwise you will encounter problems when the macro is invoked prior to an else, i.e.

#define RUN_ONCE(runcode) \
do \
{ \
    static bool code_ran = 0; \
    if (!code_ran) \
    { \ 
        code_ran = 1; \
        runcode; \
    } \
} while (0)

e.g. if you have a situation like this:

if (do_foo)
    RUN_ONCE(foo());
else
    printf("do_foo is false\n");

without a do ... while (0) this will expand to:

if (do_foo)
    {
        static bool code_ran = 0;
        if (!code_ran)
        { 
            code_ran = 1;
            foo();
        }
    }; // <<< syntax error here !
else
    printf("do_foo is false\n");

But using the do ... while (0) form we get:

if (do_foo)
    do
    {
        static bool code_ran = 0;
        if (!code_ran)
        { 
            code_ran = 1;
            foo;
        }
    } while (0); // <<< no syntax error here !
else
    printf("do_foo is false\n");
\$\endgroup\$
  • \$\begingroup\$ care to show example where there will be an error? i dont really understand the point of this while loop. \$\endgroup\$ – CocoCoder Aug 31 '11 at 18:20
  • \$\begingroup\$ @CocoCoder: OK - see edited answer for example \$\endgroup\$ – Paul R Aug 31 '11 at 21:32
  • \$\begingroup\$ See also: codereview.stackexchange.com/questions/1679/… \$\endgroup\$ – Paul R Aug 31 '11 at 21:43
  • \$\begingroup\$ i see, that will be catched via compiler though, so no problems \$\endgroup\$ – CocoCoder Sep 21 '11 at 21:25
  • \$\begingroup\$ @CocoCoder: yes, it will generate a compile error - that's the whole point - better to write the macro properly so that it can be used anywhere without generating any errors. \$\endgroup\$ – Paul R Sep 21 '11 at 21:32
5
\$\begingroup\$

I prefer this over the macro-with-arguments version.

#define RUN_ONCE                                        \
    for (static bool _run_already_ = false;             \
         _run_already_ ? false : _run_already_ = true;) \
/***/

The above is not thread-safe, though. To make it thread safe, one could use std::atomic<>, but only if using C++11 and above.

#include <atomic>

#define RUN_ONCE                                       \
    for (static std::atomic<int> _run_already_(false); \
         !_run_already_.fetch_or(true);)               \
/***/

I used int rather than bool as the std::atomic<> template parameter because the standard doesn't seem to require the bool specialization to implement the fetch_or() method.

Either way, it can be used like this:

RUN_ONCE
{
    printf("Hello only once!\n");
    /* Other statements follow */
}

Or like this:

RUN_ONCE printf("Hello only once!\n");

It also supports returning directly from the RUN_ONCE body:

// Returns false the first time it's invoked,
// then it always returns true
bool test_run_once() {
    RUN_ONCE return false;

    return true;
}
\$\endgroup\$
3
\$\begingroup\$

Why are you putting code that determines correct usage of the function external to the function.

Rather than that why not put the code inside the function then it can never be used incorrectly.
The top priority is maintenance and re-use. You should write your code so that it can not be used incorrectly.

Thus rather than:

#define RUN_ONCE(runcode) \
{ \
    static bool code_ran = 0; \
    if(!code_ran){ \ 
        code_ran = 1; \
        runcode; \
    } \
}

void init_random()
{
    srand(time(NULL));
}

int main()
{
    RUN_ONCE(init_random())
}

You can do this:

void init_random()
{
    static bool code_ran = 0;
    if(!code_ran)
    {
        code_ran = 1;
        srand(time(NULL));
    }
}

int main()
{
    init_random();
}
\$\endgroup\$
  • \$\begingroup\$ the whole point of my macro was that i dont have to write that what you did in init_random(). it is so general thing i have repeated it in dozen of functions, and i always forget what was the variable name i used there (i want consistent code...), also the point of my macro is that it uses curly brackets AROUND the whole code, making the static variable only visible in that part of code i assigned there with my macro, thus i have very little possibility of any variable name collisions, and this eliminates my problem of "thinking variable name". also, how exactly is yours easier to maintain ? \$\endgroup\$ – CocoCoder Aug 27 '11 at 10:46
  • \$\begingroup\$ the mistake you did in your example was to assume that i would do the check outside of the init_random() function btw, i would do it: void init_random(){ RUN_ONCE(srand(time(NULL))); } instead. \$\endgroup\$ – CocoCoder Aug 27 '11 at 10:48
  • \$\begingroup\$ @CocoCoder: Do you actually think that using the macro actually makes the code clearer? In my opinion the answer is no. But if you (and the people that maintain the code) think it is perfectly readable then it is OK. Maintainability is they key as most code will be in maintenance mode a lot longer than it is in development. I think using a macro to save a few keystrokes is not worth the effort and extra cost of maintenance. You are basically swapping easy of programming for more difficult maintenance. \$\endgroup\$ – Martin York Aug 27 '11 at 16:52
  • \$\begingroup\$ can you tell me why is my macro harder to maintain ? what could possibly go wrong in it and so on. \$\endgroup\$ – CocoCoder Aug 27 '11 at 23:19
  • \$\begingroup\$ @CocoCoder: Its a macro. Thus it is not de-buggable. Macros are not pat of the language (they are simple text replacement tools) thus neither the compiler or debugger understand them. But its not just that you are adding a whole layer of un-needed indirection into the code that makes it hard to read. Also your macro is written badly. If it looks like a function call I expect it to behave like one (and thus be usable wherever a normal function is encountered (your code will fail in several situations). see here: codereview.stackexchange.com/q/1679/507 \$\endgroup\$ – Martin York Aug 27 '11 at 23:36
1
\$\begingroup\$

Macros are generally a bad choice because some mad sod always finds a bad way to use them...

RUN_ONCE(}else{ or_not();)
\$\endgroup\$
  • \$\begingroup\$ that argument could apply to anything. \$\endgroup\$ – CocoCoder Sep 21 '11 at 21:26
  • \$\begingroup\$ and the code is wrong too . . . ;) \$\endgroup\$ – dex black Apr 5 '12 at 3:52

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.