Re: Remove Moment::as_scheme (issue 346070043 by d...@gnu.org)
Reviewers: Dan Eble,
Message:
On 2018/06/13 21:11:38, Dan Eble wrote:
LGTM and I would not fault you if you chose to push this immediately
after
receiving independent confirmation that it builds.
Shrug. I have no dependencies on this one. It can sit in its branch
until the shoe drops. I just found it when using Moment as a sort of
template for a new Transform class and dug around finding what it's even
being used for.
Description:
Remove Moment::as_scheme
It's a remnant of some design long abandoned.
Please review this at https://codereview.appspot.com/346070043/
Affected files (+1, -14 lines):
M lily/include/moment.hh
M lily/moment.cc
Index: lily/include/moment.hh
diff --git a/lily/include/moment.hh b/lily/include/moment.hh
index
c29032f99dc6f56d32fc087b2dcecfaccda7fe16..8a6136eb2870a430da3584363cf912a8b21f99ab
100644
--- a/lily/include/moment.hh
+++ b/lily/include/moment.hh
@@ -56,12 +56,9 @@ public:
bool to_bool () const;
I64 den () const;
I64 num () const;
- /*
-Deliver a copy of THIS as a smobified SCM
- */
+
string to_string () const;
static int compare (Moment const &, Moment const &);
- SCM as_scheme () const;
};
IMPLEMENT_ARITHMETIC_OPERATOR (Moment, +);
Index: lily/moment.cc
diff --git a/lily/moment.cc b/lily/moment.cc
index
20c1f58a0b51cdef2e0c5e42e16c4dced7014e7d..d05fafc9e3755fe01a73e4ea34c70eedf0d360b1
100644
--- a/lily/moment.cc
+++ b/lily/moment.cc
@@ -58,16 +58,6 @@ Moment::print_smob (SCM port, scm_print_state *) const
return 1;
}
-SCM
-Moment::as_scheme () const
-{
- return scm_list_5 (ly_symbol2scm ("ly:make-moment"),
- scm_from_int64 (main_part_.num ()),
- scm_from_int64 (main_part_.den ()),
- scm_from_int64 (grace_part_.num ()),
- scm_from_int64 (grace_part_.den ()));
-}
-
SCM
Moment::equal_p (SCM a, SCM b)
{
___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel
Remove Moment::as_scheme (issue 346070043 by d...@gnu.org)
LGTM and I would not fault you if you chose to push this immediately after receiving independent confirmation that it builds. https://codereview.appspot.com/346070043/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Issue 5336: Remove downcasting methods from Grob_array and Grob_info (issue 344010043 by nine.fierce.ball...@gmail.com)
On 2018/06/13 03:24:02, dan_faithful.be wrote: I perceive that we understand each other’s points and simply disagree. There is nothing new I want to counter with. I will just state that if a contributor were made uncomfortable by dynamic_cast, my two-pronged solution would be (1) gently encourage him to educate himself on this fundamental feature of C++, and (2) over time, rework the software to require fewer casts by preserving more type information in the internal interfaces and pushing the casts outward toward the interface with Scheme. I now understand more about the overhead that is involved in the encapsulation that I thought was desirable. Rather than an execution overhead, there is a coding overhead. For every type of dynamic cast I may want to use, I need to provide a getter method. And this just covers up a dynamic cast; there's not any reasonable error handling involved in the getter method. That's not very smart, I see now. I wholeheartedly agree with your changes. Thanks for running with this issue. Carl https://codereview.appspot.com/344010043/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Issue 5336: Remove downcasting methods from Grob_array and Grob_info (issue 344010043 by nine.fierce.ball...@gmail.com)
I am convinced by these arguments. Thank you for your patience with me. Hopefully we can get some rats taken care of. Carl https://codereview.appspot.com/344010043/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
PATCHES - Countdown for June 13th
Hello, Here is the current patch countdown list. The next countdown will be on June 16th. A quick synopsis of all patches currently in the review process can be found here: http://philholmes.net/lilypond/allura/ Push: 5337 Create Bottom contexts in a more general way - Dan Eble https://sourceforge.net/p/testlilyissues/issues/5337 http://codereview.appspot.com/339710043 5333 musicxml2ly: hidden timesigs and tempo marks with text. - Alexander Myltsev https://sourceforge.net/p/testlilyissues/issues/5333 http://codereview.appspot.com/34443 Countdown: 5340 Simplify Spacing_spanner::get_columns - Dan Eble https://sourceforge.net/p/testlilyissues/issues/5340 http://codereview.appspot.com/348940044 5338 Fix out-of-sync LilyScriptEncoding / ps script defs - Knut Petersen https://sourceforge.net/p/testlilyissues/issues/5338 http://codereview.appspot.com/347870043 5334 Use system* instead of system when invoking browser - James Lowe https://sourceforge.net/p/testlilyissues/issues/5334 http://codereview.appspot.com/336240043 Review: 5341 Urgent corrections to stencil-integral.cc (skylines) - Torsten Hammerle https://sourceforge.net/p/testlilyissues/issues/5341 http://codereview.appspot.com/341350043 New: 5343 Remove Moment::as_scheme - David Kastrup https://sourceforge.net/p/testlilyissues/issues/5343 http://codereview.appspot.com/346070043 Regards James ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Issue 5337: Create Bottom contexts in a more general way (issue 339710043 by nine.fierce.ball...@gmail.com)
This looks to me like a nice job of making the code more understandable and predictable. LGTM Carl https://codereview.appspot.com/339710043/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: musicxml2ly: handle hidden time signatures; support text+bpm \tempo marks. (issue 344000043 by a.mylt...@gmail.com)
On 2018/06/04 10:51:25, a.myltsev wrote: Removed the 'test' commits, leaving only changes to Python files. Patch counted down - please push. Alex if you do not have commit access can you attach a git-formatted patch (re-based against current master) and I can push it for you - attach it to the tracker, I will see it - https://sourceforge.net/p/testlilyissues/issues/5333/ https://codereview.appspot.com/34443/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Issue 5336: Remove downcasting methods from Grob_array and Grob_info (issue 344010043 by nine.fierce.ball...@gmail.com)
On 2018/06/13 03:24:02, dan_faithful.be wrote: On Jun 12, 2018, at 18:18, mailto:carl.d.soren...@gmail.com wrote: > > The tradeoff of having people know about dynamic casting and using it > properly needs to be matched with people not needing to know about > dynamic casting and being able to ignore it. Carl, I appreciate hearing your perspective and I’d like to continue this discussion to the point of reaching a consensus on the design. I perceive that we understand each other’s points and simply disagree. There is nothing new I want to counter with. I will just state that if a contributor were made uncomfortable by dynamic_cast, my two-pronged solution would be (1) gently encourage him to educate himself on this fundamental feature of C++, and (2) over time, rework the software to require fewer casts by preserving more type information in the internal interfaces and pushing the casts outward toward the interface with Scheme. > As I said before, I'm not asking for a reversion. I think I just have a > different tradeoff value model than you have. Clearly, and I’m sure David can contribute a third perspective, and I hope he will choose to. I've been hoping to avoid that. This attempted clean-up and some other work in progress has been about as engaging as pounding sand down a rat hole, The only thing I've found to work yet are cats and poison. Cats scale down the visibility and scale of the problem. Poison at least manages to keep the problem in the house at bay because there you got enough of a rationale to revert to second-generation poisons in-house when the damn critters have literally consumed several kg of first-generation bait (which is reasonably harmless regarding the effects on rat corpse feeders) with diminishing effect. Either require a human bothered about dealing with the problem to a sufficient degree to keep working on it. Such engagement may start with pounding sand down a rat hole. and I have only persevered in it because I considered it a step in the direction of a cleaner system in the long run. If it is valuable, then I would be willing to continue, but if that is not the general perception, The "general perception" is irrelevant as long as the general is not working on the code. However, this work includes reviews. I don't see a future for LilyPond when nobody is working on it, but the future without people bothering to review it is not an inviting prospect, either. It's too big for that to work. Dan's work here is trying to lower the amount of code lookups a typical C++ programmer and LilyPond newbie will have to deal with. It hardcodes a LilyPond code concept as a dynamic cast and little else makes sense (apart from possibly adding an assertion). I can follow his rationale that we'd want to stick in the proper type domain in the first place and want to get there eventually. At the same time, it's a distinction completely irrelevant at the Scheme level where things are dynamically typed. I readily agree that the overall performance impact is small. At the same point of time, you have to declare your types in C++ all the time and overly generic declarations, while individually causing only very little performance drain mostly irrelevant to what the dynamic Scheme type system has to go through all of the time, cause programmers wasting time on figuring out what the code is talking about here. It's bad C++ Feng Shui, distracting from more important work. I'll readily admit the effects on inner architecture for making a house welcoming but am utterly untalented in that regard. Thus I am very much content with those who actually are bothered by details enough to propose changes and argue for them to start clean up. This change here is not actually changing things as much as a bit of sorting allowing for throwing some things out later. So it's a bit pointless to argue for or against it on its own merits and/or make a molehill of the sand to be pounded into a rat hole. I sure hope that we'll get to the rats themselves eventually when we have reduced the number of holes they keep running out of. then I would rather take one step back, restore the code to an acceptable state (probably not quite a straight reversion, though), and never touch it again. If the code were in acceptable state, we'd have more programmers and fewer discussions. I can see that this is necessary at the interface between Scheme and C++, but it doesn’t have to be carried through to all the C++ internals, with each step working to determine the specific type information it requires but neglecting to propagate it to the next step. That loose approach could be continued, but it doesn’t have to be. Should it be continued? I'd prefer to have code enjoyable to work with. What this means is different at the C++ and the Scheme level. If you have to declare your types in a static type lattice, it makes sense declaring them in the manner
