PR: https://github.com/aibasel/downward/pull/281

Thanks for the review. I don't think this need more review and merged the code.

Looks great! Changing the ABORTs to errors is nice. I would say it's a minor
issue that we cannot collect all errors at once, given that these errors are
program bugs rather than user errors, so the convenience requirements are
lower.

I didn't check every single line, but from my side it's ready to merge.
Please merge unless you think it requires further review.

I'm changing the
title to help for the time when we create the next release.

I created a pull request for this. It moves the code for creating the registry
from RawRegistry::create_registry() to the constructors of Registry and
TypeRegistry. Those constructors are now private and only called from an
`instance()` method (singleton pattern with a static variable in the method).

One downside is that previously, we collected errors and reported them in one
list rather than stop at the first error. This is now split into two such lists:
one for the type registry and one for the registry. This means that if the code
has multiple errors regarding the feature and type registry, the user will first
get a list of all errors in the type registry, and only sees the registry errors
after fixing the type errors.
On the positive side, we can now include errors in
the type registry that previously were not included in this report. For example,
registering an EnumType twice previously was assumed to never happen and led to
an abort, but it did happen (making us aware of this issue). Now it is included
in the list of errors for the type registry.
I'd say over all, the trade-off is
worth the cleaner design.

I'm not sure I agree with the second point. In the current design, attempting to recreate the registry a second time by accident is a bug for sure, and I think causing a crash is preferable to having it pass silently. (Of course, making it impossible or at least have an assertion is preferable to crashing, but crashing is better than hiding the bug.)

Of course this doesn't affect what we discussed yesterday and how we should approach addressing this.

While working on issue757, we noticed that constructing the feature registry
twice causes the planner to crash because internally the construction adds enum
types to the (singleton) type registry. If those enum types are registered
twice, the type registry throws an error.

There are two aspects to this:
*
Constructing the registry twice is unnecessary: we should construct it once and
keep it around instead.
* Constructing the registry twice (even if it is not
necessary) should not crash the planner.

We also saw that the code to construct
the Registry currently lives in RawRegistry but we want to keep this class as a
dumb storage class as much as possible. We thus plan to move the functionality
into one or two factory methods on the level of the registry and type registry.