Header guard naming

Archive Blender Development
1

I was just a little too late for D7450 so @jacqueslucke suggested taking it to devtalk.

-----8<------[cut here] -----8<------

! In D7450#179813, @brecht wrote:
I suggest using __BLI_FLOAT2_HH__ for now.

Switching to #pragma once would be good I think, but we can do that globally.

since we were picking on _t being reserved for posix, i feel i can pick on this a tiny bit and i’ll start with what the proper solution ought to be edit: new insights after talking on chat

#ifndef bli_float2_hh
#define bli_float2_hh
...
#endif

Why?

well the C standard says

Reserved identifiers
The following identifiers are reserved and may not be declared in a program (doing so invokes undefined behavior):
[…]
2) All external identifiers that begin with an underscore.
3) All identifiers that begin with an underscore followed by a capital letter or by another underscore (these reserved identifiers allow the library to use numerous behind-the-scenes non-external macros and functions)

However lets pull up the C++ standard as well here

Section 17.6.4.3.2/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.12) is reserved to the implementation for any use.
— Each name that begins with an underscore is reserved to the implementation for use as a name in the global namespace.

as for #pragma once it’s not in any standard, also gcc seems to have some perf issues with it

So we end up in our current situation

#define __BLI_FLOAT2_HH__ clearly goes against 2 sets of standards, but everyone does it anyhow.
#pragma once is not in any standard, but all modern compilers support it anyhow.

which leads me to the solution posted above

On chat we looked at how chrome and llvm handled this they seemingly went with

#ifndef <PROJECT>_<PATH>_<FILE>_H_. for chrome (ie CHROME_COMMON_CAST_MESSAGES_H_ for chrome/common/cast_messages.h)

llvm seems to follow a similar scheme
#ifndef LLVM_MC_MCEXPR_H for llvm/MC/MCExpr.h

Seems like a good solution, also given the define is depended on the path we could easily script this to do it in bulk.

1 Like
2

If we do this kind of renaming and break a bunch of patches and branches, I think there needs to be a real benefit. I can see that for switching to #pragma once. But I’m not aware of the current naming causing actual problems.

3

The real benefit is we no longer violate both the c and c++ standards, that alone should be a worthy goal imho. (also clang on windows emits quite a few warns about this one in the default configuration)

I have a hardtime imagining a rename action like this causing issues for branches, let alone a bunch of them. unless there already multiple branches out there that edit the header guards i’m not aware of?

as for #pragme once it’ll move us from violating the standards to undefined behavior which i suppose is better, but also a tiny bit worse at the same time.

I imagine the rename script to function similar to make format, so if you don’t want to type out the correct header guard name, or just don’t know what you should use, toss a #ifndef fixfixfixfixfixfix at the top of your header and run the script…(if we stick to the llvm format for guards, we may not even have to make a script clang-tidy should be able to do this)

4

I’d rather leave as-is, or consider switching to #pragma once.

Note, a reason to use underscore prefix was to avoid these defines being included in auto-completion (at the time qt-creator included them IIRC) which was quite annoying.

5

I wrote a small script (P1561) that changes the include guards in the source directory to #pragma once. After running the script, you still have to run make format. There are four special cases that are excluded explicitly in the script. The full patch is available in P1562.

If we decide to use the naming scheme suggested by @LazyDodo, a slightly modified version of the script could be useful as well. However, based on the comments from @brecht and @ideasman42, it seems more likely that we move to #pragma once eventually.

We have #pragma once in our code base for quite a long time already anyway, so I assume all the compilers we care about, support it.

CC: @sergey

6

If we do this kind of renaming and break a bunch of patches and branches

Any refactor breaks patches and branches. Odds are that this specific refactor wouldn’t break THAT many things, and that it’s very simple to recover from.

think there needs to be a real benefit

Converging to a common rule we all follow everywhere is quite measurable benefit to me.

We have #pragma once in our code base for quite a long time already anyway, so I assume all the compilers we care about, support it.

At this time I am in favor of #pragam once. I do see the style guides like Chromium (aka Google) says to use old-style header guard, but it doesn’t explain why exactly.

What is the downside/drawback of #pragma once? I can only think of a rare case when one need to control things like “do not include this header directly, include FOO instead”. Is there anything else I miss?

What stops us from switching to #pragma once ?

7

The only other downside I found is when the build system copies a header file and it is included in the same translation unit twice. With #pragma once the file will still be included twice, while with our old headers guards, it is only included once (stackoverflow). However, that seems like a very niche problem and something we should not do in the first place. I’m in favor of using #pragma once as well.

8

I’m fine with using #pragma once.

9

Hard to tell what someone elses reasons is, but if i had to guess it probably be along the same lines as the the cpp core guidelines.

Which is a somewhat cryptic description as well, but as i read it, headers that were either symlinked or duplicated in the build system will now be seen as two different headers, given we would apply this change to only our code where neither of these things should be happening, I doubt it’s a big deal.

I was hoping for a more standards complaint solution for us, but this is not a hill for me to die on, given the number of people in favor of it #pragma once would be ‘fine’

10

I also prefer #pragma once if compiler support allows it.
Less risk of errors (typos), less LoC, no need for cleanup commits because the exact style wasn’t followed, etc.

Whenever I use the Qt Creator wizards to create a header, I have to manually edit the guards to match the Blender style (FOO_H -> __FOO_H__, in 3 lines), which is a little annoyance that would be nice to get rid of.

11

Personally I quite like #pragma once, but I also think it’s nice to adhere to the C/C++ standards and I don’t want to put that aside lightly.

From what I understand, there are two issues with the non-standardness:

  1. The behaviour of #pragma once can vary between compilers & platforms. From what I understand of Wikipedia this is a real issue, but mostly on the topic of what happens when the same file is available in multiple locations (because of copying, symlinking, or hardlinking). Relying on either symlinks or hardlinks isn’t really cross-platform, and I don’t think we’re using exact copies of header files in the Blender source code.
  2. The compiler may not know what #pragma once means, and silently ignore it. I don’t think this going to be any issue, as there are many compilers that support #pragma once.

All in all the concrete downsides seem to be minimal in the context of Blender’s code.

C++ Code Guideline SF.8 also says:

It injects the hosting machine’s filesystem semantics into your program, in addition to locking you down to a vendor.

Given that the compilers that support #pragma once lists so many modern compilers, I don’t think this is a practical limitation.

Using #pragma once is simpler for humans, as it’s easily copyable, and there can’t be any naming conflicts between header guards. Apart from that it can be faster (as the precompiler doesn’t have to parse the rest of the file) – I’m curious to see if this has any noticable effect on Blender’s build time.

In conclusion, I’m in favour of #pragma once. I do like sticking to standards, but in this particular case it seems that the downsides are minimal, while the upsides can be considerable. And if the downsides turn out to be bigger than we expect, we can always write a script to translate back to header guards.

12

You sure about that ?

13

Oh my…

Apart from the actual isssue, I’m always a bit afraid when a bug that’s 7 years old has status “new”.

14

good thing there was a test attached to the ticket, so it is easy to verify with a modern-ish compiler

[root@SRV inc]# g++ --version
g++ (GCC) 9.3.1 20200408 (Red Hat 9.3.1-2)
Copyright (C) 2019 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

[root@SRV inc]# time g++ -c guards-only/main.cpp

real    0m1.346s
user    0m0.141s
sys     0m1.203s
[root@SRV inc]# time g++ -c pragma-only/main.cpp

real    0m2.991s
user    0m0.922s
sys     0m2.047s

to give a fair comparison, both were ‘hot reads’

That being said, it’s a pretty dumb test, this thing parsed 10.000 (stupidly short) headers and it was 3 vs 1.3 seconds… so yeah there’s a difference, but i highly doubt it’ll make any substantial changes to the blender build time regardless of what we choose.

But technically it IS slower :slight_smile:

1 Like
15

There is one more advantage of #pragma once that I forgot to mention. This is actually sometimes mentioned as a disadvantage, but I wholeheartedly disagree with that.

The thing with header guards is that they allow other headers to detect that a certain file was #included. For example, they can change their behaviour depending on whether tls_version_1_0.h or tls_version_1_3.h was included. To me such code has a nasty smell, as such use all of a sudden makes those header guards part of the public API. Whether this is the case is determined by other header files, which can be very hard to find out.

This (in my opinion) mis-use is avoided when using #pragma once, forcing developers to make such things explicit. In the mentioned example of TLS versions, I would be much happier with an explicit #define TLS_VERSION 1.0, which can then be documented, and used in other headers.

1 Like
16

Just as a note, we don’t need to expect this to be a long term guideline I think. Once we’re ready for C++20 we should probably switch to using modules by default. No more headers then.
So whatever we use now, will probably only be the guideline for few years.

1 Like