Page 1 of 2

Correct header inclusion guards to comply with C++ standard

Posted: Mon Mar 11, 2013 1:46 pm
by antred
Hello,

This is not so much a feature request as a minor clean-up request. ;) I've noticed that most Ogre header's have inclusion guards like:
#ifndef __OGRE_BLAH_BLAH_H
#define __OGRE_BLAH_BLAH_H
The thing is, the C++ standard has this to say on identifiers containing 2 consecutive underscores:
17.4.3.1.2 Global names [lib.global.names]
1 Certain sets of names and function signatures are always reserved to the implementation:
— Each name that contains a double underscore (_ _) or begins with an underscore followed by an uppercase
letter (2.11) is reserved to the implementation for any use.
So perhaps this inclusion guards could be changed to comply with the C++ standard? I know this is just a minor matter, but it wouldn't hurt to be as standard-compliant as possible, right?

Re: Correct header inclusion guards to comply with C++ stand

Posted: Mon Mar 11, 2013 3:59 pm
by Klaim
I think it have been notified before but it wasn't worth the time to do the change. Or maybe you can take the time to provide a patch/PR?

Re: Correct header inclusion guards to comply with C++ stand

Posted: Mon Mar 11, 2013 4:30 pm
by jacmoe
The reason for that nonstandard standard is AFAIK that Visual Studio used to hide defines starting with two underscores.
It doesn't seem to do that any more, though.

IMO, it should be changed to use #pragma once instead.

AFAIK, all compilers support #pragma once. And it doesn't pollute the global namespace.

Re: Correct header inclusion guards to comply with C++ stand

Posted: Mon Mar 11, 2013 7:15 pm
by zootlewurdle
pragma once may be widely supported but it is not part of any standard. define guards are the only truly compiler agnostic solution.

Re: Correct header inclusion guards to comply with C++ stand

Posted: Mon Mar 11, 2013 7:27 pm
by saejox
<3 #pragma once

Only ancient compilers do not support it.

Re: Correct header inclusion guards to comply with C++ stand

Posted: Mon Mar 11, 2013 8:21 pm
by masterfalcon
Though it's supported in many compilers http://en.wikipedia.org/wiki/Pragma_once. I feel that it's better to maintain standard compliance, even if it's a little messier.

If you want to do a pull request to standardize the header guards that'd be great. It's a large but innocuous change and I see no reason why it would be a problem.

Re: Correct header inclusion guards to comply with C++ stand

Posted: Mon Mar 11, 2013 8:39 pm
by jacmoe
Could you name one compiler where it's not supported? :)

#pragma once is less code. It doesn't require the programmer to come up with a name for the define constant, which could potentially clash with other constants defined. (I've had that happen lots of times).
And there isn't any risk of mismatched #ifdefs.
A couple of years ago, it probably was a good idea to stick with manual conditional include guards, but not now. IMHO.

Re: Correct header inclusion guards to comply with C++ stand

Posted: Tue Mar 12, 2013 2:16 pm
by zootlewurdle
I've been working as a professional C/C++ programmer for 21 years and I've never seen a collision with header guards.
It's not hard with a library like Ogre to construct a name which should be uncommon enough to avoid problems, e.g., HH_OGRE_<MODULE>_<FILE>_HH
If you're using identically named files within any given module you need a slap and you learn your lesson (because doing so can cause confusing issues anyway).
If you're using similar guard names outside of a module or library then similarly that's a heard lesson learned not to be so silly.

Wholescale changing headers in an open source library to use a non-standard feature in something that is intended to be compiler/platform specific is simply unwise at best.

I'm trying very hard to be polite here, and probably failing.

Re: Correct header inclusion guards to comply with C++ stand

Posted: Tue Mar 12, 2013 2:45 pm
by jacmoe
I don't really give a fuck about politeness. I know programmers are notoriously anal about the smallest of issues.

You haven't presented a valid argument, only slanting. Like: "you need a slap", "I am an expert programmer (So shut up)", "learn your lesson", "lesson learned not to be so silly", "unwise at best", ...

#pragma once reduces the possibilities for bugs, and that's a really good reason to use it.

Non-standard or not.

Re: Correct header inclusion guards to comply with C++ stand

Posted: Tue Mar 12, 2013 3:25 pm
by Transporter
I can do a patch for #pragma once tonight. But I still have an open pull request wich will be updated if you don't accept it :D

Re: Correct header inclusion guards to comply with C++ stand

Posted: Tue Mar 12, 2013 4:02 pm
by jacmoe
#pragma once is unambiguous in any compiler that supports it.

It increases readability, and - even though Zootlewurdle haven't had problems with it in his 21 year long career - regular, old-fashioned include guards are error prone.
#pragma once is supported by any recent compiler worth mentioning.
Insomniac Games prefers pragma once in their code standard: http://www.insomniacgames.com/core-coding-standard/

If Ogre removes the leading underscores, then we will have issues with some of the files, like:

Code: Select all

#ifndef __GLRenderSystem_H__
#define __GLRenderSystem_H__
...
#endif
That would become GLRenderSystem_H and would probably clash with other include guards 'somewhere'.
Unless, of course, we insert a GUID into the define. Which would make it uglier, but more fail safe.

I like that #pragma once is unambiguous. ;)

I also realize that this is just as hot potato as tabs versus spaces and indentation style..

So, whatever floats the boat for the OGRE Team.
Just be careful about removing the double underscores as it might require updating to more unique defines.

Re: Correct header inclusion guards to comply with C++ stand

Posted: Tue Mar 12, 2013 7:06 pm
by Klaim
I'm not against or for pragma once. But my understanding is that it's definition IS ambiguous, it's not certain all compilers do exactly the same with it.
See: http://stackoverflow.com/questions/1695 ... o-standard
A directive like #pragma once is not trivial to define in a fully portable way that has clear an unambiguous benefits. Some of the concepts for which it raises questions are not well defined on all systems that support C, and defining it in a simple way might provide no benefit over conventional include guards.

When the compile encounters #pragma once, how should it identify this file so that it doesn't include its contents again?

The obvious answer is the unique location of the file on the system. This is fine if the system has unique locations for all files but many systems provide links (symlinks and hardlinks) that mean that a 'file' doesn't have a unique location. Should the file be re-included just because it was found via a different name? Probably not.

But now there is a problem, how is it possible to define the behaviour of #pragma once in a way that has an exact meaning on all platforms - even those that don't even have directories, let alone symlinks - and still get the desirable behaviour on systems that do have them?

You could say that a files identity is determined by its contents, so if an included file has a #pragma once and a file is included that has exactly the same contents, then the second and subsequent #includes shall have no effect.

This is easy to define and has well defined semantics. It also has good properties such that if a project is moved from a system that supports and uses filesystem links to one that doesn't, it still behaves the same.

On the downside, every time an include file is encountered containing a #pragma once its contents must be checked against every other file using #pragma once that has already been included so far. This implies a performance hit similar to using #include guards in any case and adds a not insignificant burden to compiler writers. Obviously, the results of this could be cached, but the same is true for conventional include guards.

Conventional include guards force the programmer to choose a macro that is the unique identifier for an include file, but at least the behaviour is well-defined and simple to implement.

Given the potential pitfalls and costs, and the fact the conventional include guards do work, it is not surprising to me that the standards committee didn't feel the need to standardize #pragma once.
I also found before another point about pragma once being a problem in a specific but rare case, can't find it yet though.

Anyway, I don't bother personally, I use either one or the other depending on the project and it would not be any problem to me (I think) if Ogre or any other of my current dependencies would switch to #pragma once.

If you provide patch doing it, it would be interesting to check the behaviour in unity build mode....

Re: Correct header inclusion guards to comply with C++ stand

Posted: Tue Mar 12, 2013 7:10 pm
by jacmoe
Yeah, but that topic is ancient, brother! :)
The situation was quite a bit different in 2009. I would recommend against it if someone asked me back then.
These days I bet we can't find a compiler which doesn't support it.

Re: Correct header inclusion guards to comply with C++ stand

Posted: Tue Mar 12, 2013 9:12 pm
by Transporter

Re: Correct header inclusion guards to comply with C++ stand

Posted: Tue Mar 12, 2013 9:17 pm
by Wolfmanfx
afaik i had problems with pragma once on android compilers - so i am unsure its wise to just change and than revert again when i stumble on the issuse again.
Also the code is already there why should we change a working system?

Re: Correct header inclusion guards to comply with C++ stand

Posted: Tue Mar 12, 2013 9:25 pm
by jacmoe
OpenFrameworks changed #ifdefs to #pragma once for their Android code 4 months ago.
And I found some Android code using #pragma once in code from Google. So I guess it works for Android.
What kind of compiler are we talking about there?
I thought Android was primarily Java.
But, you have a point there.

Re: Correct header inclusion guards to comply with C++ stand

Posted: Tue Mar 12, 2013 9:32 pm
by Wolfmanfx
android is not just java thx to android ndk we have many supported compiler gcc 4.3,4.6, clang as i mentioned i had to change my code during a customer project so i am not happy with such a change. This needs testing on all platforms.

Re: Correct header inclusion guards to comply with C++ stand

Posted: Tue Mar 12, 2013 9:34 pm
by jacmoe
You are right.
It is supported by the major compilers:
http://en.wikipedia.org/wiki/Pragma_once#Portability
GCC, Clang, and (of course) VC and 4 others.

And I knew about the ndk :)
Thank God for that.

Re: Correct header inclusion guards to comply with C++ stand

Posted: Tue Mar 12, 2013 9:52 pm
by Klaim
What this page doesn't say is what is exactly the algorithm implemented.

Re: Correct header inclusion guards to comply with C++ stand

Posted: Wed Mar 13, 2013 12:03 am
by madmarx
17.4.3.1.2 Global names [lib.global.names]
1 Certain sets of names and function signatures are always reserved to the implementation:
— Each name that contains a double underscore (_ _) or begins with an underscore followed by an uppercase
letter (2.11) is reserved to the implementation for any use.

So perhaps this inclusion guards could be changed to comply with the C++ standard? I know this is just a minor matter, but it wouldn't hurt to be as standard-compliant as possible, right?
This is standard. The quote is about the compiler, not the precompiler. #ifdef __ANYTHING is for the precompiler, while the quote is adressing only the compiler.

Concerning those who have never seen names collisions, I remember having such with directx with the directshow samples for example. Microsoft hid 2 files error.h with __ERROR__ defines inside. That was in 2009, and it could still be. Also I remember once or two copy paste errors on these (well it was quick to correct anyway, no big deal).

My point of view :
During long time I used #defines, but for the last 2 years I began using pragma because there are more arguments for it than against it :
1/ less error prone.
2/ vastly faster to preprocess (i remember a thread about these tests on the boost nabble). It is REALLY faster. Because most of the time, the precompiler knows that he does not need to re-open the .h (yes, that's the algorithm). Otherwise the precompiler opens the .h (<=> minimum 2 hdd accesses), and read that the precompiler-variable is already defined and does nothing with the file.
3/ easier to setup.
The case where pragma once is a problem are : when you use preprocessor programming with recurssive self inclusion of files. For example like with some of Boost.PP macros.

Now, if everything already work with #define in Ogre, I am not convinced there is so much to gain by suddenly changing the existing code base to use #pragma once; apart of course to gain some compilation speed.

Best regards,

Pierre

Re: Correct header inclusion guards to comply with C++ stand

Posted: Wed Mar 13, 2013 9:48 am
by Klaim
madmarx wrote: Now, if everything already work with #define in Ogre, I am not convinced there is so much to gain by suddenly changing the existing code base to use #pragma once; apart of course to gain some compilation speed.
If it does reduce compilation time significantly, it would be a huge improvement.

Re: Correct header inclusion guards to comply with C++ stand

Posted: Wed Mar 13, 2013 9:54 am
by CABAListic
That is unlikely. Most compilers do have special logic to deal with inclusion guards, so the difference should not be that noticable. And I'd bet you won't notice any when doing a Unity build.

Re: Correct header inclusion guards to comply with C++ stand

Posted: Wed Mar 13, 2013 10:05 am
by Klaim
That is my understanding too. That being said, it would be interesting to check the fact.
I will not be online for some days so I can't test this PR yet but will later, see if I find a difference, but as you say, I doubt it.

Re: Correct header inclusion guards to comply with C++ stand

Posted: Wed Mar 13, 2013 12:51 pm
by madmarx
And I'd bet you won't notice any when doing a Unity build.
Yes, I completely agree with you.
Most compilers do have special logic to deal with inclusion guards, so the difference should not be that noticable.
I didn't know that, but that would make sense.

Also the "include precompilation time acceleration" is very unlikely to be a major part of the whole compilation time, that is why I don't think there could be much gain anyway.
The figures where on the Boost mailing list 2-3 years ago.

Re: Correct header inclusion guards to comply with C++ stand

Posted: Wed Mar 13, 2013 3:43 pm
by jacmoe
So, the bottom line is that there is very little difference between pragma once and include guards, except that VS handles pragma once slightly more efficient than include guards (although I don't know if that's changed).
But it's definitely cleaner, and less error prone.
Include guards feels like a hack to me - I mean: it's introducing symbols for no real reason. Then again, C/C++ is full of hacks. :)