[WIP] tailored_alloc/delete: make connection_table a CRTP base class

[Quuxplusone]

trafficstars

Supersedes #229.

The way to fix -Woverloaded-virtual is to make the overload set non-virtual, and make the virtual function list non-overloaded. Also, make the virtual functions non-public, to ensure that nobody accidentally calls them directly and/or non-virtually. (This patch is a specific application of the Non-Virtual Interface Idiom, of which I am a huge fan. I didn't apply it generally to the whole codebase or even the whole class, only targeted to fix this specific problem; but I certainly wouldn't object if it started being used more widely.)

I don't think the overloading of the name relative_matrix is mechanically important, but I was too lazy to go change all the call-sites to call relative_matrixc or relative_matrixh as appropriate.

[zenorogue]

I seem to get an error in gcc 11.1.0:

locations.cpp:183:47: error: ‘degree’ is not a constant expression
  183 |   size_t num_bytes = offsetof(T, c.move_table[degree]) + degree;

[Quuxplusone]

Ah, the simple offsetof is foiled by GCC, okay. I've got a better idea anyway. This PR is now based on top of #233, so please look at that one first (and hopefully merge it as non-controversial). Then, there are 4 commits here that aren't in #233. The ultimate effect is to make c1->c().spin(d) mean exactly the same thing as c1->c.spin(d) used to mean; but without any offsetof calculations.

Ideally, @jruderman would confirm or deny that the static analyzer is happy with this approach. If this makes the static analyzer confused, then it's probably not an improvement.

[jruderman]

With these changes, I get a crash on startup in hr::geometry_information::generate_floorshapes:

• crash log with address sanitizer (probably more useful)
• crash log without address sanitizer

(16ee53e5f29953986dbe61e557db4d40c53d12ad; Apple Clang on Mac)

[Quuxplusone]

With these changes, I get a crash on startup in hr::geometry_information::generate_floorshapes:

Oh, crap. I don't get the crash myself (Clang trunk, OSX), but I agree, I see where it comes from.

heptagon modelh;
cell model;
model.master = &modelh;
modelh.c7 = &model;
model.type = modelh.type = S7;

and then later:

vector<cell> models(n);
vector<heptagon> modelh(n);

My changes make it so that when you allocate just a heptagon or a cell through normal means, e.g. on the stack, you get a "degree-zero" object, not a "degree-120" object. So that's a problem.

I'm thinking about adding a helper factory function, like

std::unique_ptr<cell, tailored_deleter<cell>>
make_tailored(int degree) {
  size_t num_bytes = T::tailored_alloc_size(degree);
  T* result = ::new (::operator new(num_bytes)) T();
  result->type = degree;
  result->c().fullclear();
  return std::unique_ptr<cell, tailored_deleter<cell>>(result);
  }

and then replacing all those cell instances with std::unique_ptr<cell, tailored_deleter<cell>> instead. That's going to make this patch even more invasive than it already is. But maybe it'll come out the other side looking easier and safer...

[zenorogue]

I do not think there is much point to hide the "spin" and "mirror" functions (it was originally h->c.spin and h->c.mirror because I decided it was easier to do it that way than to define "spin" and "mirror" shortcuts, but with the CRTP approach I think h->spin and h->mirror are nicer).