Sign in to follow this  

Is there a trickier way to write this?

Recommended Posts

I have a simple shorthand macro for debugging, which looks like this:

#define if_once(_name) static boolean _name = true; if(_name && !(_name = false))

Which I use as:

if_once(myonce) {
 ... do stuff once ...
}

The problem is that it generates a C4706 (assignment within conditional expression in VS). 

 

Here's my Neanderthal approach to the problem:

 

I really like my macro - it's convenient, especially when I need to prototype stuff fast.

I don't want to change its apparent syntax (eg whatever goes in the macro body is fair game as long as it doesn't affect the if_once(myonce) { } part, meaning that the curly braces must stay outside the macro body).

I don't want to add a warning disable-default pragma block around each use since that completely defeats the purpose of a shorthand.

I definitely don't want to disable the warning globally.

 

Therefore, since I don't want to include a curly brace in the macro body or write cryptic dual block guards, I can't think of a way to write it without an assignment in the if body.

 

Any brilliant ideas how to swindle the compiler?

 

Share this post


Link to post
Share on other sites

I hate it when an answer smacks in you in the face literally as soon as you finish writing a question on a public forum...

 

I suppose one can just write it as:

 

#define if_once(name) static boolean name = false; if(!name && ++name)

 

:wacko:

Share this post


Link to post
Share on other sites

I will not ask as to why you would want something so ugly and with such a massive bad smell in your code base, might as well store all your data in global variables as well :)

I would not be including such code anywhere near any code base I worked on and would be asking whoever added something like it to remove it and think about what they were trying to achieve and a better solution.

Share this post


Link to post
Share on other sites

I will not ask as to why you would want something so ugly and with such a massive bad smell in your code base, might as well store all your data in global variables as well :)

I would not be including such code anywhere near any code base I worked on and would be asking whoever added something like it to remove it and think about what they were trying to achieve and a better solution.

 

 

Actually, that's precisely what this is for - managing globals during brainstorming.

 

I've accumulated a substantial codebase by now, which means it's much faster and easier for me to start writing new components in-situ in order to have minimal compile times and the least possible code spread. In these circumstances I generally iterate a lot and often; in fact, for me this kind of an approach is actually less error-prone as I will have a lot fewer unused parameters and convoluted code around after I'm done sketching out the basics and feel good enough about moving the new stuff into a more established environment. For instance, I might start out by keeping everything in the render loop until the thing I want to do actually works, or I get a better idea in which case I can quickly scrap it without hunting through headers and other code files for associated clutter. In particular, something like this restricts initializing all the what would otherwise be temporary globals into a named block, which is far easier to gauge by eye.

 

Mind you, I don't keep any such code lying around after I'm done sketching up the basics, nor am I promoting using something like this in a serious production :). Instead, this allows me to blurt whatever is in my head out there faster and test it without worrying about formalizing it every step of the way. If it works, I will have a clean block of code that I know to purge.

 

Otherwise this isn't even code smell - it's fire hazard.

Share this post


Link to post
Share on other sites
Posted (edited)
#define if_once(name) static boolean name = false; if(!name && ++name)

Using the increment operator on an operand of type bool has been deprecated since at least C++03. Possibly earlier, but I do no longer keep a copy of C++98 around, so cannot check. That being said, why are you using a type boolean instead of the proper keyword? This is C++ as per the question's tag, right?

 

You could opt to use std::call_once which does what you want and is in addition thread-safe. Unless of course, you already know no threads are involved and you fear the performance implications of thread-safety (but then you can implement your own thread-unsafe version, taking the std version as a starting point). Note that using the keyword static at function scope guarantees thread-safe initialization anyway, so there is not much to fear performance-wise (you're already paying the price!).

 

The to-be-executed code can stay in-place as a lambda, just add a couple of braces.

 

Bonus points:

(1) if you later decide to refactor, and want to move the code to a separate function, it's simple. Remove captures and add params instead so the code still works. Can do that one by one, and the code still works the same. Once done, copy-paste the lambda and give it a name.

(2) you do not get ODR-violations like you will get with your macro if you ever have two instances of if_once. But I'll admit that this can probably be fixed with an extra pair of braces, or with a __LINE__ hack that gives the boolean an unique name. Still, it's one of these unexpected not-immediately-obvious surprises that make macros so nasty.

Edited by samoth

Share this post


Link to post
Share on other sites
Posted (edited)
Using the increment operator on an operand of type bool has been deprecated since at least C++03.

 

I was actually about to look into that, but since VS didn't even so much as sneeze at it, it seemed low priority.

 

 

 

why are you using a type boolean instead of the proper keyword? This is C++ as per the question's tag, right?

 

 

Because I like my syntax hilighting the way it is. The two types are equivalent.

 

 

 

Note that using the keyword static at function scope guarantees thread-safe initialization anyway, so there is not much to fear performance-wise (you're already paying the price!).

 

 

While valid points, this, everything about name clashes and the entire paragraph in general, in my case, is overthinking it. As mentioned, this is nothing more than a debugging aid while sketching out a new piece of functionality. In fact, the macro is disabled in release build.

Edited by irreversible

Share this post


Link to post
Share on other sites
Posted (edited)

Usually, compilers allow you to override that assignment-in-if warning by placing an extra pair of parens around the assignment expression. So:

#define if_once(_name) static boolean _name = true; if(_name && !((_name = false)))

I would avoid the solutions based on the “++tag” thing. ++'ing a bool has been deprecated since forever and it will be removed completely in C++17.

Edited by Oxyd

Share this post


Link to post
Share on other sites

 

Using the increment operator on an operand of type bool has been deprecated since at least C++03.


I believe it's officially dead now as of C++17, too. Meaning that you might start getting compilation errors when turning on C++17 mode with newer compilers.

You could write this with a wrapper class that has an operator bool() that mutates the state upon checking, e.g.

template <typename TagT>
struct once_wrapper {
  operator bool() {
    static bool flag = false;
    bool const ret = flag;
    flag = true;
    return ret;
  }
};

#define if_once(name) if (once_wrapper<struct _oncetag_##name>())
https://godbolt.org/g/dc2OWx

 

 

Much easier to just use a char or something.

 

I love how much hard science has been dropped in response to my prematurely posted trivial inquiry, though  :ph34r: .

 

Usually, compilers allow you to override that assignment-in-if warning by placing an extra pair of parens around the assignment expression.

 

Sadly doesn't work in VS2013.

Share this post


Link to post
Share on other sites

Much easier to just use a char or something.


Except not. Because the char will roll over eventually and so become 0 again. Your if_once will turn into if_every_256th_time :P

Also, my version is safe to use inside a control statement (if, while, for, whatever) without explicit braces. So bonus points there. :P

Share this post


Link to post
Share on other sites
Posted (edited)

 

Much easier to just use a char or something.


Except not. Because the char will roll over eventually and so become 0 again. Your if_once will turn into if_every_256th_time :P

Also, my version is safe to use inside a control statement (if, while, for, whatever) without explicit braces. So bonus points there. :P

 

 

 

No it won't. If this were the case, the bool version wouldn't work either. Recall how C++ evaluates compound conditions ;)

 

Edit: oops, that's C# there, but C++ behaves the same.

Edited by irreversible

Share this post


Link to post
Share on other sites

No it won't. If this were the case, the bool version wouldn't work either. Recall how C++ evaluates compound conditions ;)


True. That's tricky and easy to miss. :) If you use that, make sure you comment it thoroughly and why it works for future maintainers.

It still falls afoul of the second point I raised, though.

// looks correct but fails to compile
if (foo)
  if_once (bar)
    statement;
  else
    other_statement;

// because it expands to this, with indention to illustrate
if (foo)
  static char bar = 0;
if (!bar && (++bar))
  statement;
else
  other_statement;

The version I posted has no such gotchas. :)

Generally-speaking, every single macro you write that is intended to be used like a statement needs to expand to a single statement. Avoid using keyword if without binding your own else unless the macro is meant to be an if statement (yours is, of course) and avoid using for/while/do statements if it is intended to start a block, as otherwise you hijack other parts of the grammar like else/break/continue.

For some other examples in addition to the one above:

// using for-statement in a control block not meant to be a loop
#define if_once_bad() for(static bool flag = false; !flag; flag = true)
while (condition)
  if_once_bad(foo)
    break; // BAD: break won't do what you intended!

// using if-statement in a control block not meant to be a conditional
#define log_statement() if (logging_enabled)
if (test)
  log_statement() cout << "my log";
else  // BAD: binds to log_statement, not the if!
  something;

If you're going to use macros, be _exceedingly_ mindful of how it will interact with any possible code around it. If you for whatever reason can't make it actually act like a statement, make it damn clear in its name or usage that it is weird and special and might potentially break surrounding code. Name it in all caps at the very least. :)

Share this post


Link to post
Share on other sites

Create an account or sign in to comment

You need to be a member in order to leave a comment

Create an account

Sign up for a new account in our community. It's easy!

Register a new account

Sign in

Already have an account? Sign in here.

Sign In Now

Sign in to follow this