Issue 5590: Remove dead code from Dot_formatting_problem (issue 583100043 by nine.fierce.ball...@gmail.com)

2019-10-29 Thread nine . fierce . ballads 

Reviewers: ,

Description:
https://sourceforge.net/p/testlilyissues/issues/5590/

Nothing was using Dot_formatting_problem::best(), which was good
because the function that was apparently intended to find the best
configuration was not keeping track of the associated best score.

Please review this at https://codereview.appspot.com/583100043/

Affected files (+5, -36 lines):
  M lily/dot-column.cc
  M lily/dot-configuration.cc
  M lily/dot-formatting-problem.cc
  M lily/include/dot-formatting-problem.hh


Index: lily/dot-column.cc
diff --git a/lily/dot-column.cc b/lily/dot-column.cc
index  
6f553f04b8aa862a124fd4aeb853edf6396f0a31..ce81edde81e6f075bdb2a33dc40faa3a04f92a74  
100644

--- a/lily/dot-column.cc
+++ b/lily/dot-column.cc
@@ -221,8 +221,6 @@ Dot_column::calc_positioning_done (SCM smob)
 cfg.remove_collision (p);
 }

-  problem.register_configuration (cfg);
-
   for (Dot_configuration::const_iterator i (cfg.begin ());
i != cfg.end (); i++)
 {
Index: lily/dot-configuration.cc
diff --git a/lily/dot-configuration.cc b/lily/dot-configuration.cc
index  
851d8c6abca20fb9b9b8d92c5b39a05991230e97..fb1decb8ff8169f952c3137ca85440c26f2b4c3d  
100644

--- a/lily/dot-configuration.cc
+++ b/lily/dot-configuration.cc
@@ -158,7 +158,7 @@ Dot_configuration::x_offset () const
   Real off = 0.0;
   for (Dot_configuration::const_iterator i (begin ());
i != end (); i++)
-off = max (off, problem_->head_skyline_.height ((*i).first));
+off = max (off, problem_->head_skyline ().height ((*i).first));

   return off;
 }
Index: lily/dot-formatting-problem.cc
diff --git a/lily/dot-formatting-problem.cc b/lily/dot-formatting-problem.cc
index  
9a97674f9b41535c862c50a67a830ca6a8d25d07..e4ee5469c252c84cc26d2e352faa462ead6d1fc1  
100644

--- a/lily/dot-formatting-problem.cc
+++ b/lily/dot-formatting-problem.cc
@@ -18,36 +18,11 @@
 */

 #include "dot-formatting-problem.hh"
-#include "dot-configuration.hh"
 #include "skyline.hh"

-Dot_formatting_problem::~Dot_formatting_problem ()
-{
-  delete best_;
-}
-
-void
-Dot_formatting_problem::register_configuration (Dot_configuration const  
&src)

-{
-  int b = src.badness ();
-  if (b < score_)
-{
-  delete best_;
-  best_ = new Dot_configuration (src);
-}
-}
-
-Dot_configuration *
-Dot_formatting_problem::best () const
-{
-  return best_;
-}
-
 Dot_formatting_problem::Dot_formatting_problem (vector const &boxes,
 Interval base_x)
   : head_skyline_ (boxes, Y_AXIS, RIGHT)
 {
-  best_ = 0;
   head_skyline_.set_minimum_height (base_x[RIGHT]);
-  score_ = 1 << 30;
 }
Index: lily/include/dot-formatting-problem.hh
diff --git a/lily/include/dot-formatting-problem.hh  
b/lily/include/dot-formatting-problem.hh
index  
0ad1c6185fb71930f63de5e4768fe05d84969324..40eb14877acec3ff1732f625c99e949ecb322201  
100644

--- a/lily/include/dot-formatting-problem.hh
+++ b/lily/include/dot-formatting-problem.hh
@@ -4,19 +4,15 @@
 #include "skyline.hh"
 #include "std-vector.hh"

-#include 
-
 class Dot_formatting_problem
 {
-public:
+private:
   Skyline head_skyline_;
-  Dot_configuration *best_;
-  int score_;

-  void register_configuration (Dot_configuration const &);
-  Dot_configuration *best () const;
+public:
+  Skyline const &head_skyline () const { return head_skyline_; }
+
   Dot_formatting_problem (vector const &boxes, Interval base_x);
-  ~Dot_formatting_problem ();
 };

 #endif

Re: Issue 5590: Remove dead code from Dot_formatting_problem (issue 583100043 by nine.fierce.ball...@gmail.com)

2019-10-29 Thread Carl . D . Sorensen 

I'm torn on this patch.  Obviously, the current case is not good.  We
shouldn't have dead code around.  But it is possible that we should in
fact use a "best" for the dot formatting problem, and the failure to do
so may lead to some of the random formatting issues we see in our
regtests.

It's not clear to me that the optimal solution to this is to remove the
best.  It might be better to keep the best and put in a FIXME that
points out it is unused.

I did some work a few years a go on the dot_column code, but never got
it ready for submission.  I will look into my code tonight when I get
home and see how I handled the "best" issue.

My bottom line is that the current system is definitely broken.  I'm not
sure if the best fix is to remove the unused code (which will hide the
potential brokenness of the dot_column creation) or to highlight the
unused code as being broken.


https://codereview.appspot.com/583100043/

Re: Issue 5590: Remove dead code from Dot_formatting_problem (issue 583100043 by nine.fierce.ball...@gmail.com)

2019-11-01 Thread nine . fierce . ballads 

On 2019/10/29 19:38:23, Carl wrote:

My bottom line is that the current system is definitely broken.


Yes, and the thing that bugs me most is that it reallocates memory per
candidate, for no benefit.


I will look into my code tonight when I get home and see how I
handled the "best" issue.


If you need more time, I won't be offended if we keep this patch in
"countdown" a bit longer; however, I don't want the best to become the
enemy of the good.  If someone later wants to return to this, it's not
hard to revert it in git, nor is it hard to reimplement a loop to find
the best of a series of things properly.

https://codereview.appspot.com/583100043/

Re: Issue 5590: Remove dead code from Dot_formatting_problem (issue 583100043 by nine.fierce.ball...@gmail.com)

2019-11-01 Thread Carl . D . Sorensen 

On 2019/11/01 17:18:34, Dan Eble wrote:

On 2019/10/29 19:38:23, Carl wrote:
> My bottom line is that the current system is definitely broken.



Yes, and the thing that bugs me most is that it reallocates memory per
candidate, for no benefit.



> I will look into my code tonight when I get home and see how I
> handled the "best" issue.



If you need more time, I won't be offended if we keep this patch in

"countdown"

a bit longer; however, I don't want the best to become the enemy of

the good.

If someone later wants to return to this, it's not hard to revert it

in git, nor

is it hard to reimplement a loop to find the best of a series of

things

properly.


OK. let's move ahead with the patch as proposed.  If I have stuff that
uses "best", it's already not in master, and my patch can add it.

If not, it's really not doing much  (if any) good to have NOP code
hanging around.





https://codereview.appspot.com/583100043/