merge-and-shrink options mockup

[Moritz, 2010-12-09 12:18:43]

I have two questions regarding your mockup, see inline comments.

http://codereview.appspot.com/3470041/diff/1/src/search/mas_heuristic.cc
File src/search/mas_heuristic.cc (right):

http://codereview.appspot.com/3470041/diff/1/src/search/mas_heuristic.cc#newc...
src/search/mas_heuristic.cc:266: ScalarEvaluator *_parse(OptionParser &parser) {
Is it intentional that _parse is not a member of MergeAndShrinkHeuristic
anymore? Won't that lead to problems because of multiple definitions of _parse?

http://codereview.appspot.com/3470041/diff/1/src/search/mas_heuristic.cc#newc...
src/search/mas_heuristic.cc:342: static ScalarEvaluatorPlugin _plugin("mas",
_parse);
Is the intention of this to replace the centralized registration of heuristics
in planner.cc?

[Malte, 2010-12-09 17:30:07]

http://codereview.appspot.com/3470041/diff/1/src/search/mas_heuristic.cc
File src/search/mas_heuristic.cc (right):

http://codereview.appspot.com/3470041/diff/1/src/search/mas_heuristic.cc#newc...
src/search/mas_heuristic.cc:266: ScalarEvaluator *_parse(OptionParser &parser) {
On 2010/12/09 12:18:43, Moritz wrote:
> Is it intentional that _parse is not a member of
> MergeAndShrinkHeuristic anymore? 

Yes, it's intentional. This is an example of SA44 ("Prefer writing nonmember
nonfriend functions.") (See "General recommendations" on
http://www.fast-downward.org/ForDevelopers/CodingConventions.) The factory
doesn't need to know the internals of the class, and hence it shouldn't.

> Won't that lead to problems because of multiple
> definitions of _parse?

Yes, the way I wrote it, it would. I meant to make this a function that is local
to the compilation unit by using "static", but apparently I forgot.

(Since this is a rarely used feature in C++ code as opposed to C code, I should
explain for those following this at home: "static" when applied to a function
makes this function module-local. It has nothing to do with other meanings of
static such as static variables or functions or static attributes of classes.)

http://codereview.appspot.com/3470041/diff/1/src/search/mas_heuristic.cc#newc...
src/search/mas_heuristic.cc:342: static ScalarEvaluatorPlugin _plugin("mas",
_parse);
On 2010/12/09 12:18:43, Moritz wrote:
> Is the intention of this to replace the centralized registration of heuristics
> in planner.cc?

Yes and no. The plugin system is/was meant to replace centralized registration
in planner.cc, but in the case of heuristics, this replacement was already done,
i.e. in current code heuristics are not centrally registered but already use
this plugin mechanism. If you check line 19/20 of the "old" code on the left,
you see that it already contains such a plugin, so this is already fully
implemented. The only real change here is that I moved it down the file because
otherwise I would have needed to add a prototype for _parse.


The only things that planner.cc currently centrally registers are the search
engines and some of the basic evaluators. We intend to get rid of those
registrations too using the same plugin mechanism, but apparently we haven't
yet. (Actually, that surprises me -- I thought all this stuff was already gone.
I must be misremembering.)

[Moritz, 2010-12-09 18:11:00]

Understood.

http://codereview.appspot.com/3470041/diff/1/src/search/mas_heuristic.cc
File src/search/mas_heuristic.cc (right):

http://codereview.appspot.com/3470041/diff/1/src/search/mas_heuristic.cc#newc...
src/search/mas_heuristic.cc:266: ScalarEvaluator *_parse(OptionParser &parser) {
Thanks, did not know that about static.

http://codereview.appspot.com/3470041/diff/1/src/search/mas_heuristic.cc#newc...
src/search/mas_heuristic.cc:342: static ScalarEvaluatorPlugin _plugin("mas",
_parse);
Ah, I missed that. 

I could move the remaining registrations out of planner.cc, if you want - I'll
have to do a few small syntax changes in that area and in plugin.cc anyway
(since my OptionParser, unlike the existing one, is not a singleton anymore).

[Malte, 2010-12-09 18:35:48]

http://codereview.appspot.com/3470041/diff/1/src/search/mas_heuristic.cc
File src/search/mas_heuristic.cc (right):

http://codereview.appspot.com/3470041/diff/1/src/search/mas_heuristic.cc#newc...
src/search/mas_heuristic.cc:342: static ScalarEvaluatorPlugin _plugin("mas",
_parse);
On 2010/12/09 18:11:00, Moritz wrote:
> I could move the remaining registrations out of planner.cc, if you want - I'll
> have to do a few small syntax changes in that area and in plugin.cc anyway
> (since my OptionParser, unlike the existing one, is not a singleton anymore).

Yes, that would be very good. When this change is made, many #includes in
planner.cc can go away, too.