@rpavlik
What's the best way for me to test this? I cloned the repo but failed to build with:

CMake Error at CMakeLists.txt:100 (add_subdirectory):
The source directory

/home/paul/projects/testing/solvespace/extlib/libdxfrw

does not contain a CMakeLists.txt file.

-- Configuring incomplete, errors occurred!

Or if you've been running with this change long enough to have confidence we should just merge it.

FWIW: the following starts building for me (note the --recursive):

git clone https://github.com/rpavlik/solvespace -b reduce-cached-pointer --recursive
mkdir -p solvespace/build && cd solvespace/build
cmake -GNinja .. && ninja

I think this PR shouldn't be necessary--unless I'm missing something, the use of cached pointers here dates back to the initial commit. @phkahler I expect that testing will show no functional changes other than a minor slowdown.

@whitequark It's not needed unless there is an effort to change function calls from taking pointers to taking handles (which is not done here). If that's the goal this moves in that direction. If we just prefer to use hg rather than g->h that's just a style change (one I like BTW). I like to move toward the future when possible, but if that's not what this is then lets drop it. If it's moving toward something we'd like let's merge it - It looks fine.

@rpavlik what was your goal here?

It's not needed unless there is an effort to change function calls from taking pointers to taking handles (which is not done here). If that's the goal this moves in that direction. If we just prefer to use hg rather than g->h that's just a style change (one I like BTW).

I agree that the style of using handles rather than pointers is, in general, more robust. I am worried about the performance impact of pervasively using multiple handle lookups where only one would suffice--in no small part because there are only a few places where handles are invalidated. It should not matter here, but it will matter if we decide to do it everywhere.

Do you prefer using handles solely because there is less chance of invalidation, or do you have other concerns here?

I just dislike the syntax of dereferencing the pointer g->h. IMHO pointers are actually preferable but they can not be written to a file. They may also have issues with regeneration - I'm still not clear on the whole remap thing. @whitequark if it ever happens, what would be preferred for hierarchical sketches? That's a future worth moving toward.

if it ever happens, what would be preferred for hierarchical sketches? That's a future worth moving toward.

Globally, we can't move away from handles (they are deeply baked into the architecture of SolveSpace), and even if we could, we shouldn't--they serve multiple important roles. That is, they turn dangling pointers (safety error) into dangling handles (logic error); they make remapping possible; they simplify savefiles (although they are not strictly required there--we could assign transient identifiers just for savefiles, and turn them back into pointers on load).

Regarding hierarchical sketches, IMO it is far too early to talk about them. There are many steps we have to take first, and they involve a significant amount of work. For example, new file format and eliminating the global sketch would be absolutely necessary. I don't think there is any rush to replace the handle system with something because, even if we do that, we still have to deal with the legacy files--so let's solve problems in the order they arrive.

If it makes it easier for the other devs of the project, this can be dropped. I saw it as a safety thing, mostly, for the reasons @whitequark mentioned. I had recently fixed some issues where a pointer was cached, then some operation invalidated the pointers but they continued to be used. (Well, that, and it looked awfully upside-down to me to look up a pointer by handle, then immediately dereference that pointer to get the corresponding handle.) I don't remember offhand the backing structure for the lookup of handles to pointers, if it's ordered or unordered, to know the overhead. But, now that I fixed the various "stale pointer" issues, this is not really critical. Shouldn't happen again until we get another new entity :-P

I think the first function changed is good and should be taken for all the reasons above that prefer handles to pointers. The second one will experience a performance hit, but probably not perceptible so I'd just merge it all.

I don't remember offhand the backing structure for the lookup of handles to pointers, if it's ordered or unordered, to know the overhead.

I believe it's binary search.

But, now that I fixed the various "stale pointer" issues, this is not really critical. Shouldn't happen again until we get another new entity :-P

Does asan find these issues? We should already be running tests under asan on Travis, though the test coverage is low.

I think the first function changed is good and should be taken for all the reasons above that prefer handles to pointers. The second one will experience a performance hit, but probably not perceptible so I'd just merge it all.

I'm confused--isn't there just one function?

Merged with minor changes as e51fdf6. I hope that looks good to everyone.