New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[WIP] tailored_alloc/delete: make connection_table a CRTP base class #230
base: master
Are you sure you want to change the base?
Conversation
I seem to get an error in gcc 11.1.0:
|
GCC isn't happy with a simple size_t num_bytes = offsetof(T, c.move_table[degree]) + degree; so we have to keep the multiplication for now.
It's now 8 bytes bigger than before, but more type-safe and doesn't require offsetof anymore.
We don't want `c1->spin(d)` to compile, but we do want `c1->c().spin(d)` to compile. Make the CRTP inheritance private and make `->c()` the magic incantation to poke through that privateness.
6aed642
to
16ee53e
Compare
Ah, the simple 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. |
/** \brief connection table */ | ||
connection_table<heptagon> c; | ||
// DO NOT add any fields after connection_table! (see tailored_alloc) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here's the benefit of the CRTP approach. We no longer have a named c
data member that must-be-the-last-in-the-class. Instead, we have a base class trailing_connection_table
, which provides a member function c()
that returns basically (connection_table<heptagon>*)(this + 1)
.
With these changes, I get a crash on startup in
(16ee53e; Apple Clang on Mac) |
Oh, crap. I don't get the crash myself (Clang trunk, OSX), but I agree, I see where it comes from.
and then later:
My changes make it so that when you allocate just a I'm thinking about adding a helper factory function, like
and then replacing all those |
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). |
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 callrelative_matrixc
orrelative_matrixh
as appropriate.