Multiple variable definition with Clang 9

[Dwarfobserver]

When compiling with Clang 9, I got

multiple definition of `cppsort::detail::iterator_category_value<std::__1::input_iterator_tag>'
multiple definition of `cppsort::detail::iterator_category_value<std::__1::forward_iterator_tag>'
multiple definition of `cppsort::detail::iterator_category_value<std::__1::bidirectional_iterator_tag>'
multiple definition of `cppsort::detail::iterator_category_value<std::__1::random_access_iterator_tag>'

because it seems that Clang 9 does not treat template specializations of constexpr variables inline anymore. Because the library is in C++14, the specializations cannot be marked inline so I think they should be static instead.

[Morwenn]

Thanks for the report!

I thought that this variable template wasn't ODR-used since it is only used as a non-type template parameter, but apparently I'm still unable to predict ODR issues. I guess that I will do as you say and make it static in C++14, and inline when the C++17 support is sufficient.

[Morwenn]

I can also remove the std::input_iterator_tag spacialisation now that I think of it, since it doesn't make sense for a sorter to handle input iterators.

[Dwarfobserver]

You're welcome :)

Mmmh, it seems Clang allows to do a declaration of the specialized variable, and so always generates it (when not inlined): https://godbolt.org/z/NfvWzn

[Morwenn]

Interestingly enough GCC allows to forward-declare the variable template as constexpr - which I thought would be forbidden - but not Clang. I keep getting surprised by all the details ^^'

[Dwarfobserver]

Haha yes, me too !

I didn't succeed with template <class T> extern constexpr int i; 🤔
Did you write template <class T> constexpr int i; ? This one defines the variable like for int i; (C syntax 🤢).

[Morwenn]

I did try that, haha.

Apparently both compilers allow inline or __inline variables as an extension in C++14 mode, but there is no way to detect that since __cpp_inline_variables is only defined in C++17 mode. I guess that I will just put them in an anonymous namespace for cpp-sort 1.x and properly use inline variables in the 2.x branch instead of trying to work around it; I have bigger issues than a few additional symbols in the binary.

[Dwarfobserver]

Yup, I think it's the best way to do it, and static variables have good chances to be optimized away anyway. I didn't know about the inline extension.

[Morwenn]

The tests passed but I can't really test the fix with Clang 9 since it's not on Travis AFAIK. I'll close the issue for now, don't hesitate to reopen if needed :)

[Morwenn]

I just found the recent CWG2433 (the link is probably not available yet) which says:

The list of entities in 6.2 [basic.def.odr] paragraph 12 that can have multiple definitions across translation units does not, but should, include variable templates.

Apparently this DR is tentatively ready for the next meeting, and it looks like it directly concerns our issue here.

[Dwarfobserver]

Nice ! I don't have access to the link but it should correct this issue indeed :)

[Morwenn]

I gave this issue another look for the C++20 branch of the project, but it seems that the status of CWG2433 support is still unknown/undocumented:

• GCC: https://gcc.gnu.org/projects/cxx-dr-status.html
• Clang: https://clang.llvm.org/cxx_dr_status.html
• MSVC: ???

As a result I decided to be conservative and to mark all namespace-scope constexpr variables inline in the library, even the variable templates.