Collection update_if_needed reform #2 #4891
Conversation
|
@ironage perhaps you could have an initial look at this. I don't consider it finished as I think there are some performance issues that can be fixed. The port is not a 1-1 implementation as quite a few things have changed since then, but I have tried to be loyal to the spirit in the original PR. Lets discuss the general idea at some point. |
|
I like this direction because it allows for more lazy loading and potentially better performance. I also think it reads better. If I had one wish, it would be to add some asserts that the accessor is not out of date in case we forget to update something. But forgetting to update an accessor is the notorious problem we already have so there's nothing new there. |
fbc5b5d to
2c13bf0
Compare
|
@ironage I consider this ready for review. |
2c13bf0 to
396ed7a
Compare
| @@ -336,7 +334,7 @@ bool Dictionary::is_null(size_t ndx) const | |||
|
|
|||
| Mixed Dictionary::get_any(size_t ndx) const | |||
| { | |||
| update_if_needed(); | |||
| // Note: `size()` calls `update_if_needed()`. | |||
There was a problem hiding this comment.
@jedelbo Isn't that a quite unexpected side-effect judging from the name? Should it perhaps be renamed? It clearly was unexpected even from the implementers judging from the fixes from where this was called ;-)
There was a problem hiding this comment.
@bmunkholm I am not sure I understand this comment. It should not be unexpected that size() calls update_if_needed.
There was a problem hiding this comment.
It just looked like that since the callsites calling size() called update_if_needed() before calling size() :-)
What, How & Why?
This is a port of #4726 to current master. More information can be found in that PR.
☑️ ToDos