link errors with 3.2.0

[reder2000]

The following two lines cause link error (using Visual Studio + mingw64 or remote ubuntu g++12) because of multiple defined symbols :

std::ostream& (&endl)(std::ostream&) = static_cast<std::ostream& (&)(std::ostream&)>(std::endl);
std::ostream& (&flush)(std::ostream&) = static_cast<std::ostream& (&)(std::ostream&)>(std::flush);

Direct compilation (ubuntu g++ or msvc) don't trigger linker problems.

However, changing these lines to

inline static std::ostream& (&endl)(std::ostream&) = static_cast<std::ostream& (&)(std::ostream&)>(std::endl);
inline static std::ostream& (&flush)(std::ostream&) = static_cast<std::ostream& (&)(std::ostream&)>(std::flush);

seems more correct, don't hurt already working builds, and fixes the linking error.

[bshoshany]

Thanks for the bug report! I'm happy to make this small change if it fixes the issue, but first I want to understand better what the issue is exactly, since I have not encountered it myself.

I tried to reproduce this now by creating a dummy project, compiling individual object files, and then inking them, but it worked without any issues on every system I tested it in, including Ubuntu 22.04 with GCC 12.0.1.

Are you able to supply a minimal working example of the error so I can test it myself? Also, can you explain why this error happens, and why does making these variables inline static fixes it? Since they are just some variables defined in the BS namespace, I don't see why there should be any issues.

[reder2000]

Ok just run make.sh in https://github.com/reder2000/thread-pool-rr if you have g++
Fails with any compiler under any os now.
What made it succeed in the first time was my use of precompiled headers (it somehow hides mutiple definitions, but not in remote g++ or local mingw64-g++, go figure out).
And after reading a while about inline and c++17, I think that just 'inline' (without static) should be used.

[krupkat]

I have the same errors, as @reder2000 commented, adding inline should be enough.

This is what clang-tidy says:

...BS_thread_pool.hpp:40:17: warning: variable 'endl' defined in a header file; variable definitions in header files can lead to ODR violations [misc-definitions-in-headers]
std::ostream& (&endl)(std::ostream&) = static_cast<std::ostream& (&)(std::ostream&)>(std::endl);
                ^
...BS_thread_pool.hpp:41:17: warning: variable 'flush' defined in a header file; variable definitions in header files can lead to ODR violations [misc-definitions-in-headers]
std::ostream& (&flush)(std::ostream&) = static_cast<std::ostream& (&)(std::ostream&)>(std::flush);
                ^

This is the failing check:
https://clang.llvm.org/extra/clang-tidy/checks/misc/definitions-in-headers.html

[krupkat]

Here is a small example: https://godbolt.org/z/cenrrh4ex

[bshoshany]

Thanks for the information @reder2000 and @krupkat!

So the issue seems to be that if the header file is included in two different translation units, then everything in the header file will be defined separately in each unit. When the two units are linked, this will cause an ODR (One Definition Rule) violation since there is now more than one definition for these variables overall.

This does not apply to classes (as long as these conditions are satisfied) which is why the classes defined in BS_thread_pool.hpp can appear in both translation units. However, in order to allow functions and variables to be defined more than once, they must be defined as inline (and the definitions must be the same in each translation unit).

Anyway, to solve this, I think I will just move the two offending variables into the synced_stream class itself (and make them inline static since they are going to be static members of this class). It will make using them a bit more cumbersome, since you will have to write BS::synced_stream::endl instead of just BS::endl, but this seems to me to be much better organized than just declaring them as global variables (I'm a big fan of encapsulation, honestly I'm not sure why I defined these as global variables in the first place).

The fix will be released soon. I'm closing this issue meanwhile.

Also, @krupkat, which arguments are you using with clang-tidy? Because I have it set up to run automatically in VS Code, but it did not show me this error during development, and still doesn't show it even for the two examples attached here.

[reder2000]

Using global variables is not that scary. That's how it's done in the STL for endl.

[bshoshany]

Yeah, and I guess that's why I initially defined them as global variables here as well. But now I realize it's not a good idea. Consider for example someone who uses use namespace std and also use namespace BS. Then they will have conflicting definitions for endl.

[reder2000]

Whoever use namespace std deserves some kind of punishment !

[bshoshany]

Haha, I agree ;)

(Although I often use that when I teach C++, because it makes code a lot cleaner and thus easier to understand for beginners. But I would never use it in actual code.)

[krupkat]

@bshoshany

This is what I currently use:

Checks: >
  -*,
  bugprone-*,
  google-*,
  misc-*,
  modernize-*,
  -modernize-use-trailing-return-type,
  performance-*,
  portability-*,
  readability-*

Thanks for the quick replies!

[bshoshany]

Thanks, that works!

[Cyphall]

@bshoshany

Another option would be to put all the implementation stuff (and global variables) under a #ifdef BS_THREAD_POOL_IMPLEMENTATION.
This way, the user only has to define BS_THREAD_POOL_IMPLEMENTATION before including the header in one compilation unit.

This is how the stb libs work (see stb_image.h for example).

[bshoshany]

Thanks! But the general trend in C++ is to avoid using macros whenever possible, because they are notoriously prone to errors. I believe C++17 introduced the inline specifier for variables exactly so that you won't have to resort to using macros. The library you linked to is written in C, so it doesn't have that option.