Skip to content

[COMMENTS?] Fix RemoveVariable/Attribute to really remove the Variable/Attribute

Podhorszki, Norbert requested to merge github/fork/germasch/pr/rmvar into master

Created by: germasch

This PR fixes #1193 (closed), see there for some background.

This PR fixes the issue by keeping a monotonically increasing counter for the index into the variable map, so no two variables can have the same index (unless the counter wraps around, which for an unsigned int takes a long time -- but it could be changed to uint64_t if one were to worry about it.) Since we now have to keep a counter together with the map, and those are macro generated member variables for each type, I made a custom type for it, EntityMap, which is essentially a wrapper around std::map that keeps track of the last index used.

It should be noted that while I believe this PR is bug-free, applying it may expose other bugs. Since the existing (buggy) code doesn't actually remove Variables/Attributes, handles remain valid, and so access after remove causes no troubles. Now that objects actually get freed, you better don't access them afterwards.

The PR in its current shape doesn't quite follow the style guide, the extra class EntityMap is defined in core/IO.h and uses inline definitions. I can easily clean that up, though, if the PR is otherwise acceptable. It includes a test that first works (because the bug is masked by not actually removing variables), then fails after that has been fixed, and finally passes again when using the monotonic and hence unique index.

Merge request reports