[READY] Add explicit instantiation of string_id<T>::NULL_ID to make clang happy.

[kevingranade]

Fixes #18493
Fixes #20915
Obsoletes #20935

Thanks to http://stackoverflow.com/questions/39420569/how-to-fix-a-purported-lack-of-an-explicit-instantiation-declaration-when-comp and fmtlib/fmt#318 for explaining how to address the issue

[Leland]

Awesome. Does this fix #17197 as well?

[kevingranade]

I think so, to get the benefit I'll add some cleanup, thanks for asking.

[kevingranade]

After a lot more fiddling around, it turns out that

1. The warning is legit, we were doing some really sketchy things here.
2. It looks like in at least some cases we are relying on this behavior.

It looks like the intent is for the initialization of NULL_ID to be optional in order to allow individual specializations of the template to provide different strings. This arrangement was allowed because string_id::is_null() and string:id::string_id( const null_id_type & ) weren't often used, and the linker would discover if they were and throw an error.

New versions of clang have started warning about this scenario regardless of whether the member is used or not, which I can't argue with. Also it seems like there are some visibility problems, the member needs to be initialized in every compilation unit where it is used, which probably means the initilization needs to happen in header files instead of in cpp files as is happening now.

Two nice-to-haves:

1. A short initialization statement, the current one is rather verbose. (template <> const emit_id string_id::NULL_ID( "null" );)
2. A way to convince GCC and earlier versions of clang that this initialization is mandatory, if not we'll have plenty of instances of this breaking in the future.

[kevingranade]

The "simplest" thing sems to be to hoist the initialization of the respective NULL_ID instances to the header files for their classes, but this is really wordy and possibly fragile. It also prevents the use of forward declarations sometimes.

I'm wondering if I can include the null string (if any) in an explicit template instantiation statement, then we can have the NULL_ID initialization happen in the body of string_id, and possibly restrict the verbosity of the instantiation.

[Leland]

Know this is a WIP but can confirm that this makes Apple's LLVM happy 🙏

[kevingranade]

Thanks for the confirmation, put it in [WIP] because it fails to link with the latest gcc:/

[kevingranade]

So... more context since this is turning out to be incredibly problematic to address.

in src/string_id.h we have:

template<typename T> class string_id {
    template<typename S, class = typename
             std::enable_if< std::is_convertible<S, std::string >::value>::type >
    explicit string_id( S && id, int cid = -1 ) : _id( std::forward<S>( id ) ), _cid( cid ) {}
    string_id() : _id(), _cid( -1 ) {}
    // This member is the issue.
    static const string_id<T> NULL_ID;
    private:
    std::string _id;
    mutable int _cid;
}

Then in various compilation units we have something like (this is src/harvest.cpp)

template <>
const harvest_id string_id<harvest_list>::NULL_ID( "null" );

At other points we have references to NULL_ID.

The result is the kind of warning in #18493:

src/string_id.h:66:50: error: instantiation of variable 'string_id<emit>::NULL_ID' required here, but no definition is available [-Werror,-Wundefined-var-template]
        string_id( const null_id_type & ) : _id( NULL_ID._id ), _cid( NULL_ID._cid ) {}
                                                 ^
src/emit.h:14:18: note: in instantiation of member function 'string_id<emit>::string_id' requested here
        emit() : id_( NULL_ID ) {}
                 ^
src/string_id.h:154:35: note: forward declaration of template entity is here
        static const string_id<T> NULL_ID;
                                  ^
src/string_id.h:66:50: note: add an explicit instantiation declaration to suppress this warning if 'string_id<emit>::NULL_ID' is explicitly instantiated in another translation unit
        string_id( const null_id_type & ) : _id( NULL_ID._id ), _cid( NULL_ID._cid ) {}

I've tried various things to sort this out, but so far I haven't found a way to make both clang and gcc happy. Clang throws the above warning on solutions gcc is happy with, and the gcc linker complains about multiple template definition if I add the explicit instantiation declarations Clang suggests. This is the explicit declaration I'm using:

extern template const string_id<harvest_list> string_id<harvest_list>::NULL_ID;

This is the explicit specialization:

template<> const string_id<harvest_list> string_id<harvest_list>::NULL_ID = string_id<harvest_list>( "null", 0 );

[kevingranade]

Wow, just wow. This turned into a massive pain in the ass, but I finally have it sorted out.
Some combination of explicit declaration of templates, removing the code that created a special generic NULL_ID, and isolating the NULL_ID specialization statements in their own .cpp file has sorted out warning-free compilation on both gcc and clang 4.

[kevingranade]

I moved some code from various headers to .cpp files, it didn't end up being necessary, but there should be as little code in headers as possible, so it's better as it is.
Similarly, I split activity_type out into its own file, also in an unsuccessful attempt to break a deadlock between implicit template instantiation and specialization, but it's large enough for its own file, so it can stay.

[Leland]

Conflict in effect.h from #20900

[Leland]

Getting this error after that merge:

clang++  -DRELEASE -DMACOSX -DGIT_VERSION -DOSX_SDL2_LIBS -DTILES -DUSE_HOME_DIR -ffast-math -Os  -Wall -Wextra    -fsigned-char -Werror -stdlib=libc++ -std=c++11 -MMD -mmacosx-version-min=10.7 -D_THREAD_SAFE -I/usr/local/include/SDL2 -DSDL_SOUND -I/usr/local/include/SDL2 -D_THREAD_SAFE -I/usr/local/include -c src/mutation.cpp -o obj/tiles/mutation.o
src/mutation.cpp:568:22: error: instantiation of variable 'string_id<mutation_branch>::NULL_ID' required here, but no definition is available
      [-Werror,-Wundefined-var-template]
    return trait_id::NULL_ID;
                     ^
src/string_id.h:146:35: note: forward declaration of template entity is here
        static const string_id<T> NULL_ID;
                                  ^
src/mutation.cpp:568:22: note: add an explicit instantiation declaration to suppress this warning if 'string_id<mutation_branch>::NULL_ID' is explicitly
      instantiated in another translation unit
    return trait_id::NULL_ID;
                     ^
1 error generated.
make: *** [obj/tiles/mutation.o] Error 1

macOS 10.12.4, compiled with Homebrew, compilation command was make app NATIVE=osx OSX_MIN=10.7 RELEASE=0 TILES=1 LOCALIZE=0 CLANG=1 USE_HOME_DIR=1 SOUND=1

[kevingranade]

Booo >_<
It needs something like

Extern template const string_id<muration_branch> string_id<mutation_branch>::NULL_ID;

probably in mutations.h

[kevingranade]

Should be sorted out now.

[Leland]

@kevingranade can confirm; successful build!