C++ Core Guidelines for static member variables

Question

I have a private static vector in my class that keeps a pointer to all objects created from it. It's necessary as each object needs access to information from all the other objects to perform some calculations:

// Header file:
class Example {
public:
    Example();
private:
    static std::vector<const Example*> examples_;
};
// Cpp file:
std::vector<const Example *> Example::examples_ = {};
Example::Example() {
    // intialization
    examples_.emplace_back(this);
}
void Example::DoCalc() {
    for (auto example : examples_) {
        // do stuff
    }
}

clang-tidy points out that I'm violating C++ Core Guidelines, namely : "Variable 'examples_' is non-const and globally accessible, consider making it const (cppcoreguidelines-avoid-non-const-global-variables)".

Personally, I don't see the resemblance between my code and the sample code in the core guidelines, especially since the variable is inside a class and private. What would be the 'correct' way of implementing this functionality? I don't want to disable this check from clang-tidy if it can be avoided.

Manes

Maybe it has something to do with SOIF. If you have C++17, define the static member inline, and see if the warning goes away. – Reverberatory

I've been doing C++ work for a ...long time. It never occurred to me that there's something fundamentally wrong with mutable static class members. The only potential issue the document refers to is the static initialization order fiasco. This is true, but not a reason to disown all static class members. – Whiffle

I wonder what happens if you use static inline std::vector<const Example*> examples_; in the class and then remove std::vector<Example *> Example::examples_ = {}; from the cpp file. Do you still get the warning? – Sniff

@SamVarshavchik and data races, and the fact you are hiding dependancies – Recipience

With gcc, this doesn't compile because you omit const here: std::vector<const Example *> Example::examples_ = {};. But I guess it is just a typo and not your real problem. – Extrauterine

Aside: you really should have a destructor that removes this from examples_, and copy and move constructors, otherwise examples_ isn't "all objects created from it" – Recipience

@Reverberatory I was getting the warning twice (header and cpp), now it's only once – Manes

@Sniff yes but only once now, in the header. – Manes

@Recipience yes. this is just sample code to illustrate the problem. – Manes

You're still getting the warning in the header with the definition inline? Yeah, that's not correct, see Asteroids' answer below, it explains why that warning is not right. – Reverberatory

Answer 1

What you've done is fine. This is literally the purpose of class-static. Some people would recommend alternatives, for unrelated reasons, which may be worth considering… but not because of anything clang-tidy is telling you here.

You've run into clang-tidy bug #48040. You can see this because it's wrong in its messaging: the vector is not "globally accessible", at least not in the sense of access rules, since it's marked private (though it's globally present across translation units, which is fine).

Your code doesn't relate to the cited core guideline.

Calandracalandria

Ooh, nice find with the bug report :) – Reverberatory

thanks! I'll just disable it in the .clang-tidy file for now then – Manes

I'd call this a bug in the core guidelines. static data members have a very similar problems to global variables – Recipience

@Recipience Not a bug :). It's already in the to-do section, see the link in my updated answer. – Athodyd

@WernerHenze I imagine that clang-tidy bug will either be closed as not-a-bug/wont-fix, or it'll be left open until that proto-rule gets fleshed out – Recipience

@WernerHenze It's a bug until it's not. – Calandracalandria

Note that the mentioned bug was fixed in August 2023, so it should not be a problem in newer versions of clang-tidy. Although clang-tidy 18.0.0 still seem to warn so I am unsure exactly what version the fix is included in. – Histamine

Answer 2

A possible solution is to force each client that accesses Example::examples_ to go through a function. Then put examples as a static variable into that function. That way the object will be created the first time the function is called - independent of any global object construction order.

// Header file:
class Example {
public:
    Example();
private:
    std::vector<const Example*>& examples();
};
// Cpp file:
std::vector<Example *>& Example::examples()
{
    static std::vector<Example *> examples_;
    return examples_;
};
Example::Example() {
    // intialization
    examples().emplace_back(this);
}
void Example::DoCalc() {
    for (auto example : examples()) {
        // do stuff
    }
}

Of course if you are sure that you have no problem with global objects and are sure that no other global object is accessing Examples::examples_ during its construction, you can ignore the warning. It is just a guideline, you don't need to follow it strictly.

As Asteroids With Wings noted the guideline I.2 does not apply to your code. But please note that the CoreGuidelines intend to ban static members as well, see To-do: Unclassified proto-rules:

avoid static class members variables (race conditions, almost-global variables)

Athodyd

The guideline actually says nothing about this case. – Calandracalandria

(I do agree that your proposed alternative is safer code.) – Calandracalandria

@AsteroidsWithWings Good point, the words don't mention static class members, but these suffer from the same initialization order problems as global variables. I'll open an issue on the CppCoreGuideline repo. – Athodyd

Thanks. Always good to learn the proper way to do something. I realize the code is not thread-safe but It'll most likely never have more than a single thread. + the calculations in each object are synchronized through an external mechanism. – Manes

Answer 3

Personally, I don't see the resemblance between my code and the sample code in the core guidelines

You have a single variable that is accessible to every thread, hidden from users of Example. The only difference to an ordinary global variable is that it is private, i.e. you can't use the name Example::examples_ to refer to it outside of Example.

Note

The rule is "avoid", not "don't use."

The "correct" way of implementing this functionality might be how you have it, but I strongly suggest you rework "each object needs access to information from all the other objects to perform some calculations" so that you pass a std::vector<const Example*> to where it is needed, having kept track of all the relevant (and especially alive) Examples where they are used.

Alternative: [...] Another solution is to define the data as the state of some object and the operations as member functions.

Warning: Beware of data races: If one thread can access non-local data (or data passed by reference) while another thread executes the callee, we can have a data race. Every pointer or reference to mutable data is a potential data race.

Recipience

"The only difference to an ordinary global variable is that it is private." But that's a fundamental difference that goes right to the heart of the cited guideline. – Calandracalandria

@AsteroidsWithWings there are still data-race and action-at-a-distance problems. They are localised to Example, but still present – Recipience

Regardless, clang-tidy is using an inapplicable rule to complain, and that's what this question is about. This has actually been raised on the LLVM issue tracker before. – Calandracalandria

What sort of races do you envisage, actually? If they're localised to Example then that's the case for any local variable, and again has little to nothing to do with the C++ core guidelines. – Calandracalandria

@AsteroidsWithWings The same races as for a scope unconstrained global variable. The races don’t change, just how much code can potentially cause it. You’re of course right that clang-tidy overly generalises the C++ core guidelines rule but it’s still a valid warning. – Malpractice

@AsteroidsWithWings Two threads initialise an Example without synchronisation -> data race, program is UB. – Recipience

@Recipience Right, and we can pick other ways to make such a class break with the introduction of threads, since it has no mutual exclusion at all. That's a massive slippery slope argument though and doesn't have anything to do with this question! :) – Calandracalandria

@AsteroidsWithWings That guideline explicitly mentions the problem of data races. – Recipience

@AsteroidsWithWings it's all "non-local data" Access control only controls the name Example::example_. It can still be passed around in references – Recipience