Issue836

Title Don't use global object for predefinitions
Priority feature Status resolved
Superseder Nosy List cedric, jendrik, malte, patfer, salome
Assigned To salome Keywords
Optional summary

Created on 2018-09-20.11:14:03 by salome, last changed by salome.

Files
File name Uploaded Type Edit Remove
lama-actioncosts-new.txt salome, 2018-09-20.17:33:19 text/plain
lama-actioncosts-old.txt salome, 2018-09-20.17:33:16 text/plain
lama-no-actioncosts-new.txt salome, 2018-09-20.17:33:12 text/plain
lama-no-actioncosts-old.txt salome, 2018-09-20.17:33:03 text/plain
Messages
msg7700 (view) Author: salome Date: 2018-09-21.10:30:11
The issue is merged.
msg7669 (view) Author: malte Date: 2018-09-20.19:26:46
It looked ready to me. :-) I left some small comments. Apart from these small
things, this looks ready to merge without another round of review. Very nice code!
msg7662 (view) Author: malte Date: 2018-09-20.17:59:07
So the pull request is ready for review?
msg7658 (view) Author: salome Date: 2018-09-20.17:33:03
I incorporated the comments on the pull request and here. I also ran two tasks
(blocks 7-2 and woodworking-opt08/p02) to see whether storing the Predefinitions
in iterated search works correctly. I append the 4 outputs; aside from the
obvious (time/memory/paths...), and some shuffled "new best heuristic value"
output, the outputs are identical.
msg7654 (view) Author: malte Date: 2018-09-20.16:43:58
All sounds good! As just discussed, the solution for #1 looks good, or at least
is consistent how the "reparsing" of the search engines is otherwise handled for
iterated search. In another context, we discussed that in the long run, the
option parser should probably create factories that can create objects to do the
actual heavy computations, rather than directly creating the objects that do the
heavy computations. In this world, hopefully the reparsing in iterated search
would become obsolete, solving this problem.

Regarding #2, this has something to do with the fact that parsing vs. finding
out which options are needed are handled by the same mechanism. I think for this
"help mode" of the option parser you can use an empty predefinitions object.
msg7646 (view) Author: salome Date: 2018-09-20.14:54:00
I made a first implementation and created a pull request:

https://bitbucket.org/salome_eriksson/downward-issues/pull-requests/2/issue836/diff

The general idea is that the Predefinitions object is created (and also dies) in
parse_command_line_aux. It is passed as a reference for inserting
predefinitions, and a const reference is stored in OptionParser so we can access
its objects when building the search engine. There are however currently two
somewhat ugly hacks:
 - IteratedSearch needs an actual copy (which is stored in the class itself),
since it parses new configs after parse_command_line_aux is called and thus
needs the Predefinitions object to live longer.
 - DocStore/DocPrinter needs an OptionParser in order to fill the docs for
PluginInfos. As far as I can tell this OptionParser would not need the
predefinitions, but with the current Implementation we cannot create an
OptionParser without it.

I think the first point is still acceptable since we need the Predefinitions to
actually survive longer. We should in any case run some tests to see whether the
new implementation works correctly in this case.
The second point I would very much like to avoid. One option could be to change
the const reference to a Pointer; then we could also pass a NullPointer to the
OptionParser in the fill_docs method. But then we always need to check for
NullPointers when actually using the object. Alternatively we could create a new
(empty) Predefinitions object in just this part of the code and pass this to the
OptionParser.
msg7622 (view) Author: salome Date: 2018-09-20.11:14:03
Currently, the object storing all predefintions in the option parser is a
singleton which is always accessed with its global instance method. We want to
get away from this and instead pass the object as a method parameter.

This is a subissue of issue588
History
Date User Action Args
2018-09-21 10:30:11salomesetstatus: in-progress -> resolved
messages: + msg7700
2018-09-20 19:26:46maltesetmessages: + msg7669
2018-09-20 17:59:07maltesetmessages: + msg7662
2018-09-20 17:33:19salomesetfiles: + lama-actioncosts-new.txt
2018-09-20 17:33:16salomesetfiles: + lama-actioncosts-old.txt
2018-09-20 17:33:12salomesetfiles: + lama-no-actioncosts-new.txt
2018-09-20 17:33:03salomesetfiles: + lama-no-actioncosts-old.txt
messages: + msg7658
2018-09-20 16:43:58maltesetmessages: + msg7654
2018-09-20 14:54:00salomesetmessages: + msg7646
2018-09-20 11:16:41salomesetnosy: + jendrik
2018-09-20 11:15:21salomesetnosy: + malte, cedric, patfer
2018-09-20 11:14:03salomecreate