I took care of the comment and merged.

PS: Adding more modern clang and gcc versions to the buildbot would be welcome
too, of course!

(Except for the comment I made on bitbucket, this looks good to merge.)

I left one comment on bitbucket.

The pull request is definitely an improvement. The lack of virtual destructor in
ConstraintGenerator is actually a serious bug that could have caused crashes or
other undefined behaviour, so it's great to see it fixed. (So this is not just a
change to make a particular compiler happy, it's an actual bug that needs fixing.)

It would be very nice to catch such things automatically in the future, and
options like -Weffc++ sound good. I understand Florian's concern in combination
with -Werror, though. Perhaps we should consider supporting building in two
different modes, with and without -Werror, have -Werror off by default, but
enable it in some or all buildbot configurations. That way we get stricter
checking, but don't have to worry about breaking other people's builds for no
good reason. On the other hand, forcing -Werror on our users has caught code
issues in the past, so there are also downsides. Perhaps it would be enough to
tell people clearly that we're interested in hearing about any warnings they get.

Regarding the unused registry attribute, if this were a more
performance-critical class, I would argue that we should only include the
attribute in the class in a debug build, but for this particular class it's
probably not worth bothering with the complications of conditional compilation.

We talked about the "registry" member and agreed to keep the test and therefore 
the member. Malte, can we merge this?

The changes look fine to me. I only had one question about initializing the
iterator of StateRegistry. It involves a friend declaration and an a field that
is only used in debug mode to assert that iterators of different registries are
not mixed. We could simplify the code by giving up that check but I don't know
if that would be worth it.

The -Weffc++ flag sounds interesting but this sentences from the documentation
sound like including the wrong header could break our build:

> When selecting this option, be aware that the standard library headers do not
> obey all of these guidelines; use ‘grep -v’ to filter out those warnings.

I made a pull request at https://bitbucket.org/jendrikseipp/downward/pull-requests/101 .

We could think about using "-Wnon-virtual-dtor" to automatically catch the virtual-
destructor-related errors in the future (or maybe even the -Weffc++ flag wich entails -
Wnon-virtual-dtor, https://gcc.gnu.org/onlinedocs/gcc/C_002b_002b-Dialect-Options.html). 
Alternatively, we could upgrade the clang version on the buildslave or add a new 
buildslave. We currently test clang-3.5. clang-6.0 detects both the virtual destructor 
and the unused "registry" variable errors, while clang-5.0 doesn't find either of these 
errors.

Emil reported that the build is failing when using AppleClang 10.0.0.10001044 and 
provided the attached patch.