Malte's test issue 122

[karpase, 2010-11-15 20:02:42]

http://codereview.appspot.com/3110041/diff/1/src/search/general_eager_best_fi...
File src/search/general_eager_best_first_search.cc (left):

http://codereview.appspot.com/3110041/diff/1/src/search/general_eager_best_fi...
src/search/general_eager_best_first_search.cc:275: 
This can't be right!!!!! :-)

http://codereview.appspot.com/3110041/diff/1/src/search/general_eager_best_fi...
File src/search/general_eager_best_first_search.cc (right):

http://codereview.appspot.com/3110041/diff/1/src/search/general_eager_best_fi...
src/search/general_eager_best_first_search.cc:259: 
Who wrote this garbage ? :-)

[Malte, 2010-11-15 20:09:46]

Added a few comments mostly on formatting (not terribly important, but worth
saying once).

http://codereview.appspot.com/3110041/diff/1/src/search/landmarks/h_m_landmar...
File src/search/landmarks/h_m_landmarks.cc (right):

http://codereview.appspot.com/3110041/diff/1/src/search/landmarks/h_m_landmar...
src/search/landmarks/h_m_landmarks.cc:4: static LandmarksGraphPlugin
landmarks_graph_new_plugin(
The class should be called LandmarkGraphPlugin.

http://codereview.appspot.com/3110041/diff/1/src/search/landmarks/h_m_landmar...
src/search/landmarks/h_m_landmarks.cc:608: << std::endl;
I wonder why uncrustify does this. This is not what we want, and there's an
explicit setting to align on "<<". Will try to look into it eventually.

http://codereview.appspot.com/3110041/diff/1/src/search/landmarks/h_m_landmar...
src/search/landmarks/h_m_landmarks.cc:1066: option_parser.add_int_option("m",
&m, "m (as in h^m)");
Does add_int_option have support for communicating the default value (m = 2) to
the user?

http://codereview.appspot.com/3110041/diff/1/src/search/landmarks/h_m_landmar...
src/search/landmarks/h_m_landmarks.cc:1069: end++;
Very small thing, but can't hurt to mention: we're using ++end, not end++, these
days because postfix-increment can be inefficient. (It won't be for plain ints,
but it doesn't hurt to standardize on the prefix option, since this is always
efficient.) I think this is in Sutter and Alexandrescu's rule 9: "Don’t
pessimize prematurely".

(I know there's a ton of old code that breaks this convention.)

http://codereview.appspot.com/3110041/diff/1/src/search/landmarks/h_m_landmar...
File src/search/landmarks/h_m_landmarks.h (right):

http://codereview.appspot.com/3110041/diff/1/src/search/landmarks/h_m_landmar...
src/search/landmarks/h_m_landmarks.h:79: static LandmarksGraph *create(const
std::vector<string> &config, int start,
Our convention regarding qualifying the std:: namespace is: 

In source files, use "using namespace std;" and drop the std:: prefix (to avoid
becoming crazy).

In header files, don't use "using namespace std;" so that the namespace of every
user of the header is not polluted. This means we need std::string here, not
string. (This is in Sutter & Alexandrescu, but I don't know the number from
memory.)

http://codereview.appspot.com/3110041/diff/1/src/search/landmarks/lama_ff_syn...
File src/search/landmarks/lama_ff_synergy.cc (right):

http://codereview.appspot.com/3110041/diff/1/src/search/landmarks/lama_ff_syn...
src/search/landmarks/lama_ff_synergy.cc:30: LandmarksGraph::LandmarkGraphOptions
op;
Does this line do anything useful? Why and what? Needs a comment.

http://codereview.appspot.com/3110041/diff/1/src/search/landmarks/lama_ff_syn...
src/search/landmarks/lama_ff_synergy.cc:87: LandmarksGraph *lm_graph =
OptionParser::instance()->parse_lm_graph(config,
Would be good to wrap this after the "(" before "config", then the rest of the
parameters would nicely fit on the second line.

[Malte, 2010-11-15 20:31:29]

Some more stuff. Still a bunch of files to go.

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

http://codereview.appspot.com/3110041/diff/1/src/search/option_parser.cc#newc...
src/search/option_parser.cc:110: predefined_lm_graphs.find(name) !=
predefined_lm_graphs.end();
If you just want to test that a map (or other associative container) contains
something but don't need the actual iterator if it's found, count is your
friend:

return lm_graph_map.count(name) || predefined_lm_graphs.count(name);

http://codereview.appspot.com/3110041/diff/1/src/search/option_parser.cc#newc...
src/search/option_parser.cc:196: } catch (ParseError e) {
"Throw by value, catch by reference." (=> Sutter & Alexandrescu). So should be
"catch (ParseError &e)".

Also affects other catches, of course, but this is unrelated to this patch. Can
someone open a general refactoring issue that mentions stuff like this?

http://codereview.appspot.com/3110041/diff/1/src/search/option_parser.h
File src/search/option_parser.h (right):

http://codereview.appspot.com/3110041/diff/1/src/search/option_parser.h#newco...
src/search/option_parser.h:81: typedef LandmarksGraph *(*LandmarksGraphFactory)(
=> "LandmarkGraphFactory"

http://codereview.appspot.com/3110041/diff/1/src/search/option_parser.h#newco...
src/search/option_parser.h:121: bool dry_run);
dry_run has a default argument in some places, but not in others, I think.

If that's the case, I'd suggest getting rid of the default everywhere.

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

http://codereview.appspot.com/3110041/diff/1/src/search/planner.cc#newcode65
src/search/planner.cc:65: } else if (arg.compare("--lmgraph") == 0) {
I'd prefer to call this "--landmarks". It's about which landmarks and orderings
to compute. That, if, how, and why we store them in a graph is none of the
user's business. ;-)

(Besides, I'm not sure if the actual "landmark graph" concept is actually all
that useful. The landmarks and orderings are, but viewing them as a graph may be
unhelpful -- e.g. because it conflates different kinds of orderings and then
gives rise to oddities like cycles that are probably not really oddities if you
look at the orderings from a more appropriate angle.)

[Malte, 2010-11-15 20:50:18]

Sent remaining comments (mostly very small stuff).

Let me know when you've got the new revision done and I'll upload an updated
patch. (Since I started this one on Rietveld, it's probably better if I also
update it. In general, it of course makes more sense for the developer to
initiate the review.)

http://codereview.appspot.com/3110041/diff/1/src/search/landmarks/h_m_landmar...
File src/search/landmarks/h_m_landmarks.cc (right):

http://codereview.appspot.com/3110041/diff/1/src/search/landmarks/h_m_landmar...
src/search/landmarks/h_m_landmarks.cc:4: static LandmarksGraphPlugin
landmarks_graph_new_plugin(
The object name should follow the class name: h_m_landmarks_plugin

http://codereview.appspot.com/3110041/diff/1/src/search/landmarks/landmark_co...
File src/search/landmarks/landmark_cost_assignment.h (right):

http://codereview.appspot.com/3110041/diff/1/src/search/landmarks/landmark_co...
src/search/landmarks/landmark_cost_assignment.h:21: private:
Drop private: at the top of the class definition; it's a no-op.

http://codereview.appspot.com/3110041/diff/1/src/search/landmarks/landmark_co...
File src/search/landmarks/landmark_count_heuristic.cc (right):

http://codereview.appspot.com/3110041/diff/1/src/search/landmarks/landmark_co...
src/search/landmarks/landmark_count_heuristic.cc:39: cerr << "Reasonable
orderings can not be used for admissble heuristics" << endl;
"can not" => "cannot"

Or maybe better "are not supported" because I actually think they *can* be used
if used properly. :->

http://codereview.appspot.com/3110041/diff/1/src/search/landmarks/landmark_co...
src/search/landmarks/landmark_count_heuristic.cc:336:
option_parser.add_bool_option("alm",
"alm" => "action_landmarks"

http://codereview.appspot.com/3110041/diff/1/src/search/landmarks/landmarks_g...
File src/search/landmarks/landmarks_graph.cc (right):

http://codereview.appspot.com/3110041/diff/1/src/search/landmarks/landmarks_g...
src/search/landmarks/landmarks_graph.cc:66: use_external_inconsistencies(false)
{
Is use_external_inconsistencies respected? The code below doesn't seem to check
it. Why is is set to false by default; isn't it actually used by default? It it
set anywhere?

http://codereview.appspot.com/3110041/diff/1/src/search/landmarks/landmarks_g...
src/search/landmarks/landmarks_graph.cc:1351:
lm_graph->read_external_inconsistencies();
Shouldn't this check if use_external_inconsistencies is true? But I guess it
can't if it's static. Hmm... maybe I'm confused.

http://codereview.appspot.com/3110041/diff/1/src/search/landmarks/landmarks_g...
src/search/landmarks/landmarks_graph.cc:1389:
option_parser.add_bool_option("use_action_landmarks",
This option should have the same name as in the other file (where it was called
"alm". This long name is a good one.)

http://codereview.appspot.com/3110041/diff/1/src/search/landmarks/landmarks_g...
File src/search/landmarks/landmarks_graph_rpg_exhaust.cc (right):

http://codereview.appspot.com/3110041/diff/1/src/search/landmarks/landmarks_g...
src/search/landmarks/landmarks_graph_rpg_exhaust.cc:9: static
LandmarksGraphPlugin landmarks_graph_new_plugin(
Shouldn't this be called "landmarks_graph_exhaust_plugin"? (Not that the name
matters...)

http://codereview.appspot.com/3110041/diff/1/src/search/landmarks/landmarks_g...
File src/search/landmarks/landmarks_graph_rpg_search.cc (right):

http://codereview.appspot.com/3110041/diff/1/src/search/landmarks/landmarks_g...
src/search/landmarks/landmarks_graph_rpg_search.cc:5: static
LandmarksGraphPlugin landmarks_graph_new_plugin(
Should be called landmark_graph_rpg_search_plugin.

http://codereview.appspot.com/3110041/diff/1/src/search/landmarks/landmarks_g...
File src/search/landmarks/landmarks_graph_zhu_givan.cc (right):

http://codereview.appspot.com/3110041/diff/1/src/search/landmarks/landmarks_g...
src/search/landmarks/landmarks_graph_zhu_givan.cc:16: static
LandmarksGraphPlugin landmarks_graph_new_plugin(
Should be galled landmark_graph_zhu_givan_plugin.