2

disclaimer: this question is about prevention of unintended naming collisions, and make sure the following code fail to compile/link.

[edit] actually I'd be happy with something preventing this to compile/link, or something that solves this, like anonymous namespaces. But anonymous namespaces aren't supposed to go inside headers.

// Class1.h
// --------
namespace SomeLargeLib {
struct S {
  int a;
  S() { a = 1; }
};
}

// Class1.cpp
// ----------
#include "Class1.h"

void foo() { SomeLargeLib::S s; }

// Class2.h
// --------
namespace SomeLargeLib {
struct S {
  int a;
  S() { a = 2; }
};
}

// Class2.cpp
// -----------
#include "Class2.h"

int main() {
  SomeLargeLib::S s;
  return s.a; // returns 1 !!
}

What happens here is that the ctor S::S has two inline definitions, so the linker is allowed to assume all definitions are the same and pick any one. Note that moving the S::S ctors outside of the classes results in a linker error as they are no longer inline.

Anyway, since we shouldn't have anonymous namespaces in headers (see C++ Core Guidelines), what should be done to prevent such ODR violations? A linker error would be perfect but the standard says no diagnostic is needed for ODR violations.

ThreeStarProgrammer57
  • 2,906
  • 2
  • 16
  • 24
  • Is your question how to detect such violations, or how to achieve two homonymous classes? – Quentin Feb 16 '22 at 18:16
  • 2
    Just don't put classes in the global namespace with clashing names? – Alan Birtles Feb 16 '22 at 18:17
  • I don't want to achieve homonymous class, I just assume a large code base can have such unintended collisions. For instance a helper `Point` class meant to be used "locally" inside the header, but then another header does the same and happens to be in the same namespace. – ThreeStarProgrammer57 Feb 16 '22 at 18:21
  • @AlanBirtles, right, but having that in a namespace doesn't change the problem. let me edit my question – ThreeStarProgrammer57 Feb 16 '22 at 18:23
  • Either use the second class inside a function scope or namespace? –  Feb 16 '22 at 18:24
  • Use descriptive names, if you have similar classes in two places refactor them into a single class and put it in a single header – Alan Birtles Feb 16 '22 at 18:25
  • @DrewDormann, so Point must be a nested class? hmmm, but then Point can't be a template, nested template classes aren't allowed IIRC – ThreeStarProgrammer57 Feb 16 '22 at 18:27
  • @ThreeStarProgrammer57 No problem having class templates nested in other classes, only local class templates (e.g. inside a function) are not allowed. – user17732522 Feb 16 '22 at 18:28
  • @AlanBirtles the question isn't about how to fix it, it's about preventing this to happen in the first place (as it can lead to very nasty hidden bugs), using some good practice that would ensure a linker error in this case. – ThreeStarProgrammer57 Feb 16 '22 at 18:33
  • 1
    the good practice is to use descriptive class names and or namespaces – Alan Birtles Feb 16 '22 at 18:35
  • @KamilCuk hmmm, "to prevent bugs from happening, don't write bugs". I wonder why warnings exist then, there's no need for them, just write correct code. – ThreeStarProgrammer57 Feb 16 '22 at 18:35
  • I may have misinterpreted this question -- are you asking how to ensure that this ODR violation **fails** to compile / link? – Drew Dormann Feb 16 '22 at 18:37
  • @AlanBirtles, it doesn't guarantee anything, if the solution is to rename a local `Point` class to `PointForThisPurpose` and hope that no one else used _that_ name, you're saying c++ is broken. – ThreeStarProgrammer57 Feb 16 '22 at 18:39
  • 1
    @DrewDormann yes, I'm sorry if my question was unclear – ThreeStarProgrammer57 Feb 16 '22 at 18:39
  • Yep, c++ is "broken", it leaves you plenty of scope for shooting yourself in the foot, you have to be careful and have good coding standards to help prevent problems and comprehensive unit tests to detect them when they happen anyway – Alan Birtles Feb 16 '22 at 18:41
  • For functions one would use 'static'. https://stackoverflow.com/questions/6034654/static-keyword-useless-in-namespace-scope For classes? No idea. C++ is broken. – Aleksander Bobiński Feb 16 '22 at 18:41
  • You say the question is about avoiding naming collisions then reject the (good and correct) advice people give you as to how to avoid name collisions in c++. So yes, for you and you alone, c++ is broken. – Taekahn Feb 16 '22 at 22:20
  • @Taekahn in big c++ projects there is a need for a practice that guaranties you won't get odr violations. Anon namespaces solve this problem for cpp files. But they aren't supposed to go inside headers. When using templates you have to put code inside headers. If you have to manually make sure a name isn't already used in a whole namespace each time you name a class, this is bad. – ThreeStarProgrammer57 Feb 17 '22 at 00:11
  • @ThreeStarProgrammer57 Or you don't know how to properly use namespaces. My project is 1.3 million lines of code. 220k statements. 26k functions. 3k classes across some 3k files. Somehow, ZERO ODR violations. Or is that not a "big" project? – Taekahn Feb 17 '22 at 00:15
  • @Taekahn well you know that odr violations can happen silently, with no diagnostic whatsoever, right? so I'm not sure how you can be so confident that you have ZERO ODR violations. – ThreeStarProgrammer57 Feb 17 '22 at 00:20
  • Silent ones are uncommon. Compilers are more than happy to scream at you about anything and everything they can. But sure, maybe a silent one exists. So sad too bad. I can only care about 2 things: Reported Compile/link time issues and runtime issues. If neither of those are present, the code is good. There are no compile issues and no run issues, so if there is a violation, so be it. If it ever _does_ cause an issue i'll find it and smack it down. – Taekahn Feb 17 '22 at 00:29
  • @Taekahn odr violation is UB. You can have plenty UB in a program without getting any bugs. Maybe because you fixed all the UB that happened to cause bugs, maybe because you're lucky. But those are landmines, any trivial change to code / compiler flags / optimization level can trigger a bug. I think that it's agreed that UB is bad. Now, I agree that if you only have to fix the bug and recompile it's not that big of a deal. But when you actually release software to paying customers, that's another story. – ThreeStarProgrammer57 Feb 17 '22 at 00:41
  • Well, then, i would suggest taking @AlanBirtles advice, which i see as the correct answer to your question as you presented it, as well as following the practices we do (namespaces aside), which is running your program through multiple versions of multiple compilers, as well as running it through asan, tsan, ubsan, msan, have a suit of unit tests with as much practical coverage as possible as well as a suit of integration tests, along with a static analyzer. If after all that, you're still losing sleep over the thought of potential bugs in your program, no one can help you. – Taekahn Feb 17 '22 at 01:01
  • @Taekahn I'm all for good practice, but aside from "use namespaces" and "use more descriptive names" I didn't really see much. Now, genuine question, how exactly should I use namespaces so that I don't have to search for all code in a namespace each time I name a class inside it? – ThreeStarProgrammer57 Feb 17 '22 at 01:34
  • There is no hard and fast rule, so a lot of it is up to preference. I personally find using namespaces that mirror the project folder structure useful, but some people put all their files in one directory. I can link you to the google standards guide on picking proper namespaces: https://google.github.io/styleguide/cppguide.html#Namespaces There might be better resources, but i'm not aware of it. – Taekahn Feb 17 '22 at 01:43
  • The google standards mention one useful trick: putting things that are internal to a file inside a `[filename]_internal` namespace. Otherwise it says mostly "trivial" things about namespaces. That trick is better than nothing, but that's hardly a solution to me. Anyway, thanks for your comments. – ThreeStarProgrammer57 Feb 17 '22 at 01:59

1 Answers1

2

ODR violation are extremely hard to detect automatically, that's why it is ill formed no diagnostic required.

However, if your goal is to prevent and detect such cases, simply use modules with a strong ownership. The model completely prevent most (if not all) ODR violations that are hard to detect.

Example:

class1.ixx:

export module class1;
namespace SomeLargeLib {
export struct S {
  int a;
  S() { a = 1; }
};
}

class2.ixx:

export module class2;
namespace SomeLargeLib {
export struct S {
  int a;
  S() { a = 2; }
};
}

test1.cpp:

import class1;
import <iostream>;

void fun() {
    SomeLargeLib::S a;
    std::cout << a.a; // 1
}

test2.cpp:

import class2;
import <iostream>;

void fun() {
    SomeLargeLib::S a;
    std::cout << a.a; // 2
}

And the great thing is that you could link test1.cpp and test2.cpp together in the same binary. No collision ever happen.

However, a compiler error is emitted for test3: test3.cpp:

import class1;
import class2; // error: name SomeLargeLib::S already exists, can't import both

Those use cases are supported in MSVC already today.

Guillaume Racicot
  • 39,621
  • 9
  • 77
  • 141