arximboldi / immer

Postmodern immutable and persistent data structures for C++ — value semantics at scale
https://sinusoid.es/immer
Boost Software License 1.0
2.51k stars 185 forks source link

Fix MSVC compile error C2059 #261

Closed colugomusic closed 1 year ago

colugomusic commented 1 year ago

The following code from your documentation:

using map_t = immer::map<std::string, int>;
struct part{};
struct whole {
    part p;
    map_t m;
};

lager::state<whole> state = lager::make_state(whole{});
lager::cursor<part> part_cursor =
    state.zoom(lager::lenses::attr(&whole::p));
lager::cursor<map_t> map_cursor =
    state.zoom(lager::lenses::attr(&whole::m));
lager::cursor<int> int_cursor =
    map_cursor.zoom(lager::lenses::at("foo"))
    .zoom(lager::lenses::or_default);

Will produce the following error in MSVC when attempting to zoom in on the map element:

.../immer/detail/hamts/node.hpp(300,26): error C2059: syntax error: ''symbol''
(followed by many more lines of error message)

Qualifying the explicit destructor call with T:: satisfies the compiler.

arximboldi commented 1 year ago

Thank you!

colugomusic commented 1 year ago

Unfortunately this fix seems to be required for MSVC C++20 mode, but breaks when compiling in C++17 mode. In C++17 the old vp->~T() compiles fine. No idea if it's a compiler bug or what. An #ifdef on the standard version might be the only thing we can do to support both. What do you think?

arximboldi commented 1 year ago

Weird... C++17 also introduces std::destroy_at, maybe you can try that? (We need to ifdef that also anyway for C++14 support though).

colugomusic commented 1 year ago

More findings (you won't like it.) I have some old code that looks like this:

struct SnapshotBlock {
    ID id{};
    engine::state::BlockState state;
};
struct SnapshotLane {
    ID id{};
    immer::vector<ID> blocks;
};
...
struct Snapshot {
    struct Data {
        ...
        immer::table<SnapshotBlock> blocks;
        immer::table<SnapshotLane> lanes;
        ...
    };
    ...
};

This works fine in C++17 mode with the old version of immer I was using (006461c). After switching to C++20 mode I get the error C2059. After updating immer to the latest version something truly remarkable happens...

immer/detail/hamts/node.hpp(300,29): error C2523: 'sn::SnapshotBlock::~SnapshotLane': destructor tag mismatch (compiling source file Z:\dv\blockhead\back\src\blocks\base\base_active_block.cpp)

This is referring to that explicit destructor call on line 300: vp->T::~T(); Somehow MSVC has got completely confused and now believes that T is both SnapshotBlock and SnapshotLane. The error goes away if I comment out the places where I insert into the tables elsewhere in the code. I'll try my best to create a minimal reproduction case but Microsoft's compiler is either really broken or the latest version of immer is doing something really bad. Either way I don't really trust the fix I made in this PR anymore.

colugomusic commented 1 year ago

Weird... C++17 also introduces std::destroy_at, maybe you can try that? (We need to ifdef that also anyway for C++14 support though).

This appears to be the answer! I will submit another pull request but I want to do a lot more testing first and see if I can get both of my projects building.

arximboldi commented 1 year ago

Cool, thanks!