1

Is it correct to define a global std::string in a header file like this :

namespace Colors
{
    static const std::string s_blue = "Blue";
}
Aminos
  • 754
  • 1
  • 20
  • 40
  • looks okay, did you encounter any problems with that? – mch Jul 26 '22 at 15:10
  • Doing it in a header file might result in multiple definitions. – Mark Ransom Jul 26 '22 at 15:18
  • @MarkRansom Note that it's "static", meaning it'll have internal linkage so each translation unit will have it's own instance. – Staz Jul 26 '22 at 15:19
  • Yes, it's perfectly fine but why not use `constexpr std::string_view s_blue = "Blue"` instead? – Staz Jul 26 '22 at 15:20
  • @Staz sure that means it will probably compile without errors, but it's less than optimal. And if one piece of code tries to modify it there could be unexpected consequences. – Mark Ransom Jul 26 '22 at 15:25
  • @Staz I'd only be wary of that solution if they are on a language older than C++17, since that could end up violating ODR. Otherwise agreed. – Rogue Jul 26 '22 at 15:28
  • @mch no, I found it in an old code base and I wonder if this kind of code is correct or not (symbol duplication). – Aminos Jul 26 '22 at 16:57

2 Answers2

4

Correct?

Yes. static here means that each translation unit (TU) that includes this header will have a unique object. So there isn't 1 s_blue object for the whole project, but one for each TU.

However since it's const all objects will be equal, so for the user it doesn't make a difference.

Recommended?

Maybe, depends.

The big downside is that you create multiple objects and all of them are initialized at program startup.

Another possible downside is the access pattern. If the program assesses s_blue from multiple TUs in quick succession it could trash the cache.

However for certain programs and scenarios this could be the best option or at least "good enough". For instance (spoiler for next paragraph) in a header only library or before C++17.

Are there better alternatives?

Yes, but they cannot always be used.

extern

Make it an extern declaration in the header and in just 1 TU have the definition.

// constants.hpp
extern const std::string k_blue;

// constants.cpp
#include "constants.hpp"
const std::string k_blue = "Blue";

This is clearly superior in terms of performance over OP. However you can't use it in a header only lib.

Even in a normal program static could be preferred as the ease of declaring and defining the constants in just one place could be seen as outweighing the performance downsides in specific projects.

constexpr inline string_view C++17

A clearly better alternative if it's available (since C++17) is string_view. It's very light weight but has all the amenities of a modern container (unlike raw const char[]). And since you already are in C++17 you should make it constexpr inline:

constexpr inline std::string_view k_blue = "Blue";

inline const std::string c++17

If for whatever reason you need std::string over std::string_view you can inline the variable for better optimization:

inline const std::string k_blue = "Blue";
bolov
  • 72,283
  • 15
  • 145
  • 224
  • I like how this managed to get 2 upvotes despite describing a non-existent `external` keyword. – user3840170 Jul 26 '22 at 16:02
  • @user3840170 it's obviously a typo. I've corrected it to `extern`. A compiler would not have upvoted and would have given a error, but we humans often untedsarand the meaning or intention even in the presence of a typo. – bolov Jul 26 '22 at 16:28
1

Note that static in namespace level means: "symbol is visible only for current translation unit."

Also const at namespace level defaults to static so this keyword is not needed.

So every time you will include this file you will have new instance of this variable. Liker will not complain that there is multiple instances of this symbol.

If you are using C++17 it would be better to use std::string_view and constexpr:

namespace Colors
{
    constexpr std::string_view s_blue = "Blue";
}
Marek R
  • 32,568
  • 6
  • 55
  • 140