supported clang-cl

[durswd]

This PR makes tests and compile passed on clang-cl (clang on Windows)

[erichkeane]

I'm not sure you'd want to do this, clang-cl has the same integer sizes as MSVC, so we'd want the unsigned long overload as well here, right?

[durswd]

to suppress this error

call to member function 'saveValue' is ambiguous

in json.hpp

[jdonald]

Ok correction: in our org we never came across this even though our json.hpp looks the same. We're using clang-cl version 8.0.0. I wonder if @durswd has a different configuration whereby sizeof(long) is coming out as 8?

[erichkeane]

clang-cl supports declspecs, so I'd imagine this is useful as well.

[durswd]

declspec causes this error on clang9

cereal::detail::StaticObject<cereal::detail::bind_to_archives<PolyDerivedLA, cereal::detail::(anonymous namespace)::polymorphic_binding_tag> >' must have external linkage when declared 'dllexport'

[durswd]

this error can be reproduced on test_polymorphic

[jdonald]

We (myself + @fnabulsi + @bmcdb) got the identical error and had to make the same change, although I acknowledge this may warrant more investigation.

We are testing cereal only with static linking so it wouldn't surprise me if there's more work to ensure that clang-cl can properly build Cereal as a DLL.

[jdonald]

I was about to file a pull request then found this. We came up with byte-for-byte identical changes so fully approve here. @fnabulsi @bmcdb. Edit: Okay our changes are not identical, please see other comments.

We also were running an older version that lacked the defined(__clang__) && defined(_MSC_VER) case around line 560 of rapidjson.h, but what's in trunk is sufficient there now, so apparently rapidjson was ahead on clang-cl support while cereal just needs these three additional fixes.

[jdonald]

Edit: this line 719 in polymorphic_impl.hpp seems to be fine, not requiring any __clang__ qualification.

      #if defined(_MSC_VER) && !defined(__INTEL_COMPILER)

The CEREAL_BIND_TO_ARCHIVES() errors we were getting earlier were due to a different local change while testing one of the other three.

(Unedited) Overall any reason we get different results compared to @durswd is a combination of issues: using a different version of clang-cl, have an older fork of of cereal, and we use different features of the library which affects which templates get exercised.

You can take a look at our comparable changes here: master...jdonald:clang-cl

[AzothAmmo]

So what's the verdict here? Is this PR sufficient for supporting clang? Apparently we can build against clang on appveyor, if anyone wants to figure that out.

[jdonald]

The two unresolved concerns were:

1. The saveLong(unsigned long) part
2. CEREAL_DLL_EXPORT

For 1) I just did a test with the saveLong part conditioned in our own codebase, and I can confirm it at least doesn't hurt. I suspect this one not specifically about a long being 4 or 8 bytes, but some other MSVC cl.exe-specific weirdness.

For 2) I think it would take more work to validate if Cereal builds and runs properly as a DLL with clang-cl, but it would still be an improvement if we got the static lib clang-cl build working alone. Overall this PR only makes things better for clang-cl while changing nothing with cl, so I recommend merging it as.

[durswd]

saveLong error causes when it is compiled as 64bit. it is no problem when it is compiled as 32bit.

Instead of it

this changes uppresses this error

//! MSVC only long overload to current node
void saveValue( unsigned long lu ){ saveLong( lu ); };

->

//! MSVC only long overload to current node
void saveValue( unsigned long lu ){ saveLong( lu ); };
void saveValue( long lu ) { saveLong(lu); };

It seem that Overloading is failed on clang-cl with x64

[InBetweenNames]

The main issue with DLL support is that a tag in an anonymous namespace polymorphic_binding_tag is used as the template argument for StaticObject, which means internal linkage. So, that it works on MSVC at all is actually a compiler bug -- unless MSVC treats all anonymous namespaces as actually having the same "name" internally in the compiler, which would permit exporting.

Think about it: each TU gets it's own polymorphic_binding_tag that is different from the tag in all other TU's, yet this class is marked dllexport, which also appears in each TU. You'd effectively have a bunch of dllexported symbols that you couldn't actually access.

[InBetweenNames]

On thinking about this more: is it actually necessary to maintain the StaticObject as a dllexport?

[InBetweenNames]

OK -- I found a solution for this. It requires C++17, but essentially the following changes are made:

• polymorphic_type_impl no longer resides in an anonymous namespace
• CEREAL_BIND_TO_ARCHIVES now uses a static inline variable:

#define CEREAL_BIND_TO_ARCHIVES(...)                                     \
    namespace cereal {                                                   \
    namespace detail {                                                   \
    template<>                                                           \
    struct init_binding<__VA_ARGS__> {                                   \
        static inline bind_to_archives<__VA_ARGS__> const & b=           \
        ::cereal::detail::StaticObject<                                  \
            bind_to_archives<__VA_ARGS__>                                \
        >::getInstance().bind();                                         \
        CEREAL_BIND_TO_ARCHIVES_UNUSED_FUNCTION                          \
    };                                                                   \
    }} /* end namespaces */

The StaticObject template is still marked dllexport.

This enables the following:

• Each TU only accesses ONE StaticObject across the entire DLL
• Each DLL receives it's own StaticObject, but these are identical with correct usage of the Cereal macros (as documented)

I didn't realize this, but the reason dllexport is even used is to prevent the linker from optimizing away the static object.
It's not actually used for linkage purposes.

This change also reduces code size, as the inline static merges the definition across all TUs.

[InBetweenNames]

The same approach could be used to get the CEREAL_CLASS_VERSION macro working on MSVC 2017, where right now you can get a multiple definition error. That macro could be changed as follows (again requiring C++17)

  #define CEREAL_CLASS_VERSION(TYPE, VERSION_NUMBER)                             \
  namespace cereal { namespace detail {                                          \
    template <> struct Version<TYPE>                                             \
    {                                                                            \
      static std::uint32_t registerVersion()                                     \
      {                                                                          \
        ::cereal::detail::StaticObject<Versions>::getInstance().mapping.emplace( \
             std::type_index(typeid(TYPE)).hash_code(), VERSION_NUMBER );        \
        return VERSION_NUMBER;                                                   \
      }                                                                          \
      static inline const std::uint32_t version = registerVersion();             \
      CEREAL_UNUSED_FUNCTION                                                     \
    }; /* end Version */                                                         \
  } } // end namespaces

[InBetweenNames]

I have a proposed patch to fix this and a few other small linker-related annoyances on Windows in #657.