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 183 forks source link

Building immer::map on MSVC causes false-positive warning on popcount integer conversion #119

Closed nyanpasu64 closed 3 years ago

nyanpasu64 commented 4 years ago

https://github.com/arximboldi/immer/blob/a28718c0511ae41503b1c6ef36f4b5368fa717ba/immer/detail/hamts/bits.hpp#L86-L103

The line in question is return __popcnt64(x);. This converts a 64-bit value (between 0 and 64) to a 32-bit count_t (which has no chance whatsoever to overflow, but the compiler doesn't know that).

As a result, I get "warning C4244: 'return': conversion from 'unsigned __int64' to 'immer::detail::hamts::count_t', possible loss of data".

I think return static_cast<count_t>(__popcnt64(x)); should clear up this warning without changing behavior.

Sidenote: I think I'm not going to be using Immer, since I'm struggling with nested immutable data updates. One way would be to port the API of https://github.com/immerjs/immer to C++ but using getter/setter methods instead of properties. But I've decided to use a double-buffered document with mutexes instead.

arximboldi commented 4 years ago

@jimbo1qaz Thanks for the note. As you identified, this warning is a false positive.

Wrt. the side-note, you can use transients to achieve something similar. Note that transients have full value semantics, so they can actually be directly used in the data-model if you prefer a mutable API to the default immutable one. Also functions like update() in some of the containers are a bit similar to immerjs. I do not think how immerjs can provide value to C++, since their API is a workaround to the fact that JS supports no value-semantics. But feel free to elaborate on what your problems are and I can try help.

arximboldi commented 4 years ago

Also, recently I added a new API to Lager that may help binding together the immutable and object-oriented world: https://www.youtube.com/watch?v=e2-FRFEx8CA

nyanpasu64 commented 4 years ago

Trying to wrote immutable data updates in c++ is like pulling teeth. Let's say teeth are located at body.head.jaw.teeth[i]. So you must create an updated copy of body.head.jaw.teeth with one molar removed. Then you must copy body.head.jaw and insert the teeth with one molar removed. then copy your body.head and insert the jaw with one molar removed. Then copy body and insert the head with one molar removed.

If body was merely a nested struct, you could copy body to a local and it would become recursively mutable. But in my scenario, I had an outer struct holding a 2D immer::array and a 1D immer::map (which used to be 2D 3D as well), which had to be updated in parallel. So I had to perform a nested transient dance. I don't fully remember what I did, possibly because the code was too complex for me to understand what I was doing.

I'll look into your video when I have time.

nyanpasu64 commented 4 years ago

Is there a better way to communicate, other than replying to unrelated issues?

immer.js's API operates like:

const nextState = produce(baseState, draftState => {
    draftState.push({todo: "Tweet about it"})
    draftState[1].done = true
})

Effectively when you perform a deep mutation of an immutable structure foo[1][2][3] = 5, it creates a transient copy of foo[1][2] with an updated [3], then creates a transient copy of foo[1] with an updated [2], then creates a transient copy of foo with an updated [1]. Does Lager support this functionality?

My use case

Assume Document is a struct. foo is 3 nested immer::arrays storing integers in the end. storage is an immer::map from integers to immer::array.

Is there an easy way to start with a Document instance, then edit Document.foo[0][1][2] (3 nested immer::arrays) = 32, and insert Document.storage[32] (immer::map) = empty immer::array? I have a branch of my code which can somewhat get it done, but has a high line-of-code count and arguably lots of boilerplate, and probably longer(?) than if I were to write it in an imperative style. I haven't pushed my branch yet, but could if you want to look.

It used to be Document.storage[1][2][32] and I'd have to loop over Document.storage[1][2][x] looking for x storing empty arrays. Which made my code very complicated and confusing, and I was unable (I think) to write a working implementation using immutable structures.

Lager video/slide notes

I'm looking at https://sinusoid.es/talks/meetingcpp19/ with CSS and JS disabled (Firefox "no style" and ub0 js off), since it's the closest thing I can find to slide notes or a transcript. The text and headings reads well. The SVG code kinda works, and the images are far too big, and white objects disappear.

Actually I ended up writing a userscript to remove all <link rel="stylesheet"> tags and <anything style=""> attributes. Then I could apply my own CSS on top of that, to reduce page width and shrink images.

I found that everything in your talk, like Lager lenses and cursors, are undocumented in https://sinusoid.es/lager/ .


state<house> s seems to be a container of top-level state, of the kind you'd put in an atom.

const T& get(); s.set(put(s.get(), l, !light_on));

Whereas a cursor reads and writes to part of a state.

cursor -> reader + writer

Can you use multiple cursors to perform pseudo-imperative updates to multiple nested paths within a single Immer immutable data structure? Does the cursor/state generate transients for each modified field and all its parents? If so, this is already equivalent to what I like in immer.js. Note that I haven't tested the "Squaring the circle" or lager API yet, since it's undocumented and I haven't used Lager yet.

Do cursors only come from states? Are cursors obtained through (cursor | state)::zoom(Lens l) -> cursor<decltype(view(get(), l))>?

auto room_light_on(room_id id) {
    return attr(&house::rooms)
         | at_(id)
         | attr(&room::light_on);
}

Where does attr and at_ come from? lager lenses.hpp contains attr and at_i.

auto st = state<house>{};
auto c0 = room_component{
    st[&house::rooms][0][&room::light_on],
    ...
};

Interesting syntax.

lager::cursor<todo::item> data zug::map([](auto x) { return QString::fromStdString(x); })

What's Zug? Does it offer "map a function over an iterable" and not "hashmap data structure"?

arximboldi commented 3 years ago

Closing issue since the warning is inocuous and it can be mitigated by properly including the library as "external".

Sorry I just realized I never really replied to your last message. If some of those questions are still bothering you, please feel free to send me an email.

Cheers!

arximboldi commented 3 years ago

Also if the warning is bothering you, feel free to make a PR with the suggested workaround and I'm happy to accept it.

nyanpasu64 commented 3 years ago

Anyway I'm no longer using immutable data structures, but settled on a simplified variant of undo commands instead:

I came up with a "almost too clever" way to reduce the tedium of writing command objects. Most command APIs expect you to write a separate redo and undo method, and possibly even a separate "apply" method. Instead I created a BaseEditCommand interface where each command object subclass precomputes the new state of the altered portions of the document. When the command is applied, the undo system calls apply_swap(self: &mut dyn BaseEditCommand, &mut Document) which swaps the Document's old state with the BaseEditCommand's new state. To undo the change, you merely call apply_swap a second time, which swaps the old state from the edit command back into the document (saving the document's new state in the edit command). To redo the change, you merely call apply_swap a third time.

It has the same tradeoffs as the usual command/action pattern (richer information about what was changed, but you have to implement a new type for each operation, and might end up copying a lot of memory anyway). However it's a lot easier to implement each command (less boilerplate, somewhat lower risk of implementing undo/redo incorrectly and altering state). As a bonus, apply/undo/redo perform zero memory allocations, because they only swap data, not copy or destroy it.

One arguable downside is that you can't transplant the same command from one point in the history to another (but most undo commands in traditional applications, as opposed to VCS systems or nonlinear editors, aren't built to do that either). And I'm not sure my "can_merge" system (which merely drops commands rather than bundling or intelligently combining their contents) was a good idea, since all mergable commands must alter the same state.