Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Do not define an unnamed namespace in a header file (DCL59-CPP) #552

Closed
nlohmann opened this issue Apr 12, 2017 · 8 comments
Closed

Do not define an unnamed namespace in a header file (DCL59-CPP) #552

nlohmann opened this issue Apr 12, 2017 · 8 comments
Labels
kind: enhancement/improvement solution: wontfix the issue will not be fixed (either it is impossible or deemed out of scope)

Comments

@nlohmann
Copy link
Owner

The SEI CERT C++ Coding Standard states in DCL59-CPP:

Do not define an unnamed namespace in a header file. When an unnamed namespace is defined in a header file, it can lead to surprising results. Due to default internal linkage, each translation unit will define its own unique instance of members of the unnamed namespace that are ODR-used within that translation unit. This can cause unexpected results, bloat the resulting executable, or inadvertently trigger undefined behavior due to one-definition rule (ODR) violations.

The code currently has one unnamed namespace:

/// namespace to hold default `to_json` / `from_json` functions
namespace
{
constexpr const auto& to_json = detail::static_const<detail::to_json_fn>::value;
constexpr const auto& from_json = detail::static_const<detail::from_json_fn>::value;
}

I am not sure whether the above problem exists. Maybe @theodelrieu has an idea why we need that namespace in the first place.

@jaredgrubb
Copy link
Contributor

That explanation is odd. It acknowledges that it uses internal linkage but then cautions that it could trigger ODR violation -- which directly contradicts what internal linkage means.

@theodelrieu
Copy link
Contributor

Hi, the rationale can be found in this paper

The anonymous namespace is needed to keep the std::begin reference itself from being multiply >defined. This is from a private communicaiton with James Widman:

A constexpr reference declared at namespace scope is a variable; but
IIUC it has external linkage unless it is placed inside an unnamed
namespace.

For objects (not references): a constexpr object implicitly has a
const-qualified type, and the added const qualification at namespace
scope means that a constexpr object implicitly has internal linkage.

For references: a constexpr reference has no implicitly-added
const-qualification on its type. Even if it did, there is nothing in
the linkage rules that would implicitly give references internal
linkage.

So, it turns out that a constexpr reference at namespace scope would
have external linkage by default; so you still want the unnamed
namespace.

@nlohmann
Copy link
Owner Author

Here is the complete explanation from the guidelines:
pages.pdf

@nlohmann
Copy link
Owner Author

nlohmann commented May 8, 2017

@theodelrieu If I understand you correctly, then I can mark this issue as "won't fix", or rather a "false positive" for the underlying rule?

@theodelrieu
Copy link
Contributor

Yep, in that case we really want an unnamed namespace, "won't fix" seems appropriate

@nlohmann nlohmann added the solution: wontfix the issue will not be fixed (either it is impossible or deemed out of scope) label May 9, 2017
@nlohmann nlohmann closed this as completed May 9, 2017
@quicknir
Copy link

quicknir commented Apr 6, 2018

This is a bit of a necro, but someone sent me this link as it was relevant to a discussion we were having, and in fact what's happening here is 100% safe. The major concern is basically that that if a user defines an inline function in a header file, and that inline function uses to_json and from_json, then if the user's header is compiled in multiple TU's, you now have multiple definitions of the same inline function (which is fine) that each use a separate copy of to/from_json (which often isn't). In this case however, it is fine because a "reference initialized from a constant expression" is covered, see 6.2.2 of the C++17 draft standard: http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2017/n4659.pdf. If the references were not constexpr and not initialized from a constant expression, then while not UB on its own it would be an open invitation for UB in reasonable usage.

@theodelrieu
Copy link
Contributor

Thanks, your explanation is more digest than my copy-paste :)

@quicknir
Copy link

quicknir commented Apr 6, 2018

@theodelrieu haha well your copy-paste is quite clear on the internal linkage aspect. The key point though is that even if you get internal linkage for the reference, this can still be very bad, @jaredgrubb is only superficially correct: yes, internal linkage means that it will not directly trigger ODR violations and UB, but unless you plan to ban users from using the variable in question from their inline functions (which is a strange and unenforceable restriction), it's a very easily open door to UB.

In short, you only want to use internal linkage, header file globals for things that are both constant and constant expression initialized.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind: enhancement/improvement solution: wontfix the issue will not be fixed (either it is impossible or deemed out of scope)
Projects
None yet
Development

No branches or pull requests

4 participants