Thanks, guys! I can't dive into this deeply enough at the moment to give
meaningful comments. I've left some more comments for the parts that affect the
non-option-parser code. Apart from these parts, which I would like to sign off
on before we merge, I'm happy to see this merged once the other people on this
issue (Patrick, Jendrik, Cedric, Salomé) agree.

Some general comments on Patrick's previous message: changes that move towards a
clearer separation of responsibilities are good. I'm not sure about the
suggested new data layout and names, but if everyone is happy with the changes,
that's good enough for me. If we later think that things can be improved
further, we can improve them then.

(After an offline discussion with Jendrik)

The PluginGroupData-PluginGroupInfo & PluginTypeData-PluginTypeInfo objects
store the same kind of data (and if not modified later also the same data).
There we can reuse the struct definition. The PluginData and PluginInfo store
different data, as the former contains additional information on how to register
the plugin in the registry and the later one contains the documentation
information which is later generated. Thus, we keep those distinct structs.
=> keep structs: PluginGroupInfo, PluginTypeInfo, PluginInfo, PluginData
=> remove structs: PluginGroupData, PluginTypeData

A comment Jendrik made earlier was why do we need to give the Registry whenever
we want to call PluginInfo::fill_docs(...), PluginInfo:get_type_name(...).
During construction the registry could be stored in the PluginInfo. This is
right and currently done. But, after our discussion, the question came up, why
do we need these functions at all in there? Because, we generate the doc at a
later point (not when creating the PluginInfo object which shall contain the
doc). Why are we doing this? We think, because in the former implementation, the
Plugin creation order was unknown and required PluginGroups/Types might have
been missing. With the setup of this issue (2 phase initialization), we now know
that all required PluginGroup/Types objects exists. We could generate the
documentation directly. Thus, PluginInfo does not need both its methods anymore
(thus, there is no need to provide a registry later to it)
=> Generate documentation data when constructing the PluginInfo


We are still searching for good names for the following two classes:
PluginData: Contains all the data needed to construct later the Plugin in the
registry
RegistryData: contains all the data needed to construct later the registry
(Jendrik imagined this class is the storage for the later Registry not
containing the blueprints).

Suggestions:
RawPlugin
RawRegistry

I had a brief look at the code. If I see it correctly, we now duplicate lots of 
information, e.g., we now have both the old 

class PluginTypeInfo {
     std::type_index type;
     std::string type_name;
     std::string documentation;
[...]
};

and the new

struct PluginTypeData {
    std::string type_name;
    std::string documentation;
    std::type_index type;
[...]
};

I guess the same holds for PluginGroupInfo and PluginInfo.

Can we somehow get rid of the duplication? Maybe by storing it only in RegistryData 
and not in Registry? This would leave only plugin_factories in Registry, but maybe 
this is a good thing?

Thanks! If I have the chronology right, this means that Jendrik wanted to look
at the code now (msg7844)?

Your comments have been incorporated.

I agree with utils/strings.{h,cc}, following the existing example of
utils/collections.h. To

me, the new map functions would fit the existing utils/collections.h well,
and... drumroll... checking that file, it actually already has something called
"map_vector". So it looks like we should reuse that function rather than adding
a new map variant.

I think strings.{h,cc} (not string.{h,cc}) should be fine.

Could this not lead to misunderstandings? (therefore the _xzy part from me).
Otherwise I am fine with string.cc/h

We strive not to nest code too deeply, so I think we should store the string 
functions in src/search/utils/strings.{h,cc}.

I think we could move the string_utils.{h,cc} into the utils directory. The
option parser already uses some files from there and to prevent code duplication
I think we should keep using the utils/... files.

If at the end, the option parser is independent of the planner, but relies on
some utility classes (which are independent of the planner), I think it would be
fine. We would then ship for the option parser the "options" and the "utils"
(only required files) directory.

Store string operation in:
string_utils/string_manipulations/string_operations.{cc,h}

And the map_to_vector in:
collections.h

If they are no longer all string utilities, we could consider renaming the file
to "utils.{h,cc}" or something like that.

We could also consider moving them to one or more utilities modules outside of
the option parser if we think they are generally useful. One potential
disadvantage of this would be that it would become slightly harder to reuse the
option parser in another project by just copying the code (which is currently
not possible anyway, but we want to get there). But if these utilities modules
don't depend on anything else, I don't think there would be a big deal.

Malte's comments incorporated.

Feedback on the map_to_vector and join functions in string_utils are welcome
(and I am not sure where to put the map_to_vector, thus, I keep it in string_utils).

I stand corrected. :-) "demangle" it should be.

At least the manual entry for c++filt calls it "demangle" instead of "unmangle" 
and the former seems to be more common based on a quick google search for both 
terms.

I left some more comments on bitbucket, but they don't comprise a proper review.

> There is one question from me. three times (at two different files) we require
> the string for retrieving the real class names from the strange class names via
> c++filt -t [NAME]. Cedric wanted that the string can be obtained from a single
> position. I have put it now into errors.h/cc, because I found not better place
> for this method.

Factoring this out is good, but I would go one step further and factor out the
whole ABORT call, since in all cases the string occurs in the context of an
ABORT. This also makes it clearer that this is something that makes sense to
live in errors.h.

The slight disadvantage of this is that for ABORT to correctly report the file
name and code location where the error occurred, we have to define this as a
macro rather than a function, but given that ABORT is already macro, too, I
don't think is a real problem.

The correct C++ term for the procedure that takes the compiler-internal name and
making it human-readable is "unmangling", so I'd call the macro something like
ABORT_WITH_UNMANGLING_HINT with arguments "message" and "type_name" (or
something along those lines). While we're at it, I'd replace "correct" with
"unmangled" in the message to the user.

Cedric's comments are incorporated.
There is one question from me. three times (at two different files) we require
the string for retrieving the real class names from the strange class names via
c++filt -t [NAME]. Cedric wanted that the string can be obtained from a single
position. I have put it now into errors.h/cc, because I found not better place
for this method.

I think it's best to wait until Patrick has handled Cedric's comments.

Thanks, Cedric!

> Maybe someone else should also have a look at the code.

Any additional volunteers, then? :-)

I left some comment on bitbucket. Maybe someone else should also have a look at
the code.

Thanks, Patrick! Any volunteers for the review?

Changes ready for first ronud of reviewing:
https://bitbucket.org/PatFer/downward/pull-requests/8/issue842/diff

Errors in the Plugins are reported as
1. Errors with PluginTypePlugin<T>
1.1 Name clashes in alphabetical order
1.2 Type clashes in alphabetical order (no two different plugintypes should use
the same type T, because this T is used to identify the plugintype for a plugin)

2. Errors with PluginGroupPlugin
2.1 Name clashes alphabetical

3. Errors with Plugin
3.1 Name clashes alphabetical
3.2 Missing PluginGroups if a plugin group was defined, also alphabetical
3.3 Missing PluginTypes alphabetical

If there are errors with the PluginTypes (1.X), then error messages of type 3.3
(concerning the required PluginTypes for a Plugin) can be spurious.

Here is some relevant text from the summary of the meta-issue (issue588). I'll
remove it from there after copying it here:

==============================================================================

Plans for detecting plugin name collisions:
- Put the test that no two plug-ins have the same name in a more central place
  of the option parser code rather than in the DocStore.
- Change option parser design to do a two-phase registration of plug-ins, so
  that we can first collect all of them, then do global checks against 
  duplicates etc., and then proceed with the later parts of registration at a 
  stage where we know that all plug-ins (including type plug-ins) have been 
  initialized.
- Report name collisions of plug-ins in a well-defined order, e.g.
  alphabetically by name and for each name alphabetically listing the types.

Currently all Plugin, PluginTypes and PluginGroups are created in arbitrary
order on start up. Thus, we cannot perform test between them (e.g. that for a
Plugin which says it belongs to a PluginGroup the named group has to exist).

We want now to store the information needed for construction in a collection at
start up and after collecting them all (in our executed code) use those
information to construct the plugins and gather ALL errors.