Re: Issue 2376: Automatic footnotes on \null markups causes unexpected results (issue 5755058)
http://codereview.appspot.com/5755058/diff/1/lily/page-layout-problem.cc File lily/page-layout-problem.cc (right): http://codereview.appspot.com/5755058/diff/1/lily/page-layout-problem.cc#newcode343 lily/page-layout-problem.cc:343: return footnote_separator; Uh, Mike? You call a Scheme function for calculating a stencil, then unsmob the stencil and return a pointer to the unsmobbbed entity. The SCM expression is not kept around in any manner. How on Earth is the Scheme garbage collector supposed to guess that the Stencil for which you have retained a pointer but no referring SCM is not supposed to be collected right away? That's not just buggy code, it is also a function call interface that can't be made to work. You can't return pointers to unsmobbed entities for which no SCM reference is retained anywhere. Given the level and amount of code you write, it might be worth investing the time rereading the garbage collection chapter of the Guile manual. I'll be preparing a fix for this one as well. http://codereview.appspot.com/5755058/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Issue 2376: Automatic footnotes on \null markups causes unexpected results (issue 5755058)
On Mar 7, 2012, at 8:48 AM, d...@gnu.org wrote: Given the level and amount of code you write, it might be worth investing the time rereading the garbage collection chapter of the Guile manual. You're right that I know nothing about guile's garbage collection...it'd help to know this. I'll investigate. It'd also be good to do a little write-up for the CG w/ garbage collection dos and don'ts in the LilyPond base (i.e. what mark_smob, derived_mark, unsmob_thingee, smobbed_copy, self_scm co. all mean). Cheers, MS ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Issue 2376: Automatic footnotes on \null markups causes unexpected results (issue 5755058)
m...@apollinemike.com m...@apollinemike.com writes:
On Mar 7, 2012, at 8:48 AM, d...@gnu.org wrote:
Given the level and amount of code you write, it might be worth
investing the time rereading the garbage collection chapter of the Guile
manual.
You're right that I know nothing about guile's garbage
collection...it'd help to know this.
In a nutshell: if an SCM object is not referenced, the garbage collector
is free to throw it and its contents away.
As a reference counts:
a) getting marked using scm_gc_mark during garbage collection (mark
phase) because another referenced object does this. All Scheme data
structures do that to their members (with the exception of weak parts
of a hash table). Our own Scheme objects need to do this to their
contents via their own mark subroutines (like derived_mark).
b) getting permanently protected. Many of our own data structures
when created with new start out in protected state (since they don't
have an SCM referencing them yet, hard to avoid) and convert into SCM
via a call to unprotect () once they are properly initialized so that
calling their mark routine does not cause uninitialized garbage to be
marked.
c) having an SCM type variable in the stack referencing it. This is a
somewhat shaky operation since the compiler may optimize variables away
when they are no longer needed. If that is a concern, you can use
scm_remember_upto_here_1 and its cousins to keep the SCM variables
alive.
Strictly speaking, code like
Stencil *s = unsmob_stencil (Text_interface::interpret_markup (layout, pro
if (!s)
{
programming_error (Your numbering function needs to return a stencil.
footnote_number_markups.push_back (SCM_EOL);
footnote_number_stencils.push_back (Stencil (Box (Interval (0, 0), Int
}
else
{
footnote_number_markups.push_back (markup);
footnote_number_stencils.push_back (*s);
}
counter++;
is already borderline. From the time of unsmob_stencil, the returned
markup is no longer protected as an SCM object. If
footnote_number_markups.push_back were to cause Scheme garbage
collection to trigger, *s would likely already be trashed.
If we get a multithreaded garbage collector at one point of time, you
first need to assign the result of interpret_markup to an SCM variable
and put an scm_remember_upto_here_1 on this variable after the last use
of the unsmobbed stencil pointer. Of course, if the unsmobbed object is
part of a larger protected data structure, you need not worry all that
much. Much of the Grob * in our code base is rather laissez-faire about
protection because the grobs are usually registered somewhere else. For
example, Grob_info does not bother protecting its contents at all if I
remember correctly because it assumes that they will live longer than
the Grob_info because of unrelated reasons.
I'll investigate. It'd also be good to do a little write-up for the
CG w/ garbage collection dos and don'ts in the LilyPond base
(i.e. what mark_smob, derived_mark, unsmob_thingee, smobbed_copy,
self_scm co. all mean).
A smobbed_copy is a Scheme object with the same contents as the original
but not using it (and thus also not protecting it from collection). A
self_scm is a Scheme object referencing the original. unsmob gives a
pointer to the unwrapped object. mark_smob is called during the mark
phase of a referenced object and should in turn mark all SCM objects
that it references.
--
David Kastrup
___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Issue 2376: Automatic footnotes on \null markups causes unexpected results (issue 5755058)
Reviewers: MikeSol,
http://codereview.appspot.com/5755058/diff/1/lily/page-breaking.cc
File lily/page-breaking.cc (right):
http://codereview.appspot.com/5755058/diff/1/lily/page-breaking.cc#newcode584
lily/page-breaking.cc:584: }
On 2012/03/07 07:30:42, MikeSol wrote:
Just a C++ question - do these lines implicitly call the copy
constructor to
create footnote_stencil so that footnote_stencil is a copy of foot?
I'm not
familiar with this syntax for copying, so I just wanted to be sure
that this is
what these lines were doing.
The first line calls the assignment operator. The second line just
resets the foot pointer to point at footnote_stencil.
It would likely be cleaner to just pass a pointer to footnote_stencil
(which is constructed as a null Stencil as far as I can see) in either
case. Since the special case foot == 0 would then be ruled out in
advance (do you need the error message? Can add_footnotes_to_footer
deal with a null stencil?) one could in a more C++-like manner pass foot
in by reference instead of by pointer.
I did not want to meddle with the logic of the code: I just made sure
that it copied what was needed. If you say that add_footnotes_to_footer
can deal with getting a null stencil for starters, that's what I would
prefer using. Incidentally, the length of the axes of a null stencil
should turn out to be 0, so this would remove a few special cases as
well.
Description:
Issue 2376: Automatic footnotes on \null markups causes unexpected
results
The page layout routines passed around pointers to stencils rather
indiscriminately and worked on them rather than on stencil copies.
Sort of copy-on-write without the copy. This tries working on actual
copies where appropriate.
Please review this at http://codereview.appspot.com/5755058/
Affected files:
M lily/page-breaking.cc
M lily/page-layout-problem.cc
Index: lily/page-breaking.cc
diff --git a/lily/page-breaking.cc b/lily/page-breaking.cc
index
61a4e82f91d3098baf29e77b800a622749902522..0eee8e90a45f346a84caffeac89925da05390857
100644
--- a/lily/page-breaking.cc
+++ b/lily/page-breaking.cc
@@ -574,14 +574,21 @@ Page_breaking::draw_page (SCM systems, SCM
configuration, int page_num, bool las
p-set_property (lines, paper_systems);
p-set_property (configuration, configuration);
+ Stencil footnote_stencil;
+
Stencil *foot = unsmob_stencil (p-get_property (foot-stencil));
+ if (foot)
+{
+ footnote_stencil = *foot;
+ foot = footnote_stencil;
+}
SCM footnotes = Page_layout_problem::get_footnotes_from_lines (systems);
Page_layout_problem::add_footnotes_to_footer (footnotes, foot, book_);
if (foot)
-p-set_property (foot-stencil, foot-smobbed_copy ());
+p-set_property (foot-stencil, footnote_stencil.smobbed_copy ());
scm_apply_1 (page_stencil, page, SCM_EOL);
return page;
Index: lily/page-layout-problem.cc
diff --git a/lily/page-layout-problem.cc b/lily/page-layout-problem.cc
index
6320827c767a74b07078d328eb7041d7649f6179..82be7f204fe11223d15050ccebbf01c2d28a2dc9
100644
--- a/lily/page-layout-problem.cc
+++ b/lily/page-layout-problem.cc
@@ -162,7 +162,7 @@ Page_layout_problem::add_footnotes_to_lines (SCM lines,
int counter, Paper_book
by passing its results downstream.
*/
vectorSCM footnote_number_markups; // Holds the numbering markups.
- vectorStencil * footnote_number_stencils; // Holds translated versions
of the stencilized numbering markups.
+ vectorStencil footnote_number_stencils; // Holds translated versions
of the stencilized numbering markups.
for (vsize i = 0; i fn_count; i++)
{
if (fn_grobs[i])
@@ -176,36 +176,39 @@ Page_layout_problem::add_footnotes_to_lines (SCM
lines, int counter, Paper_book
if (!s)
{
programming_error (Your numbering function needs to return a
stencil.);
- markup = SCM_EOL;
- s = new Stencil (Box (Interval (0, 0), Interval (0, 0)),
SCM_EOL);
+ footnote_number_markups.push_back (SCM_EOL);
+ footnote_number_stencils.push_back (Stencil (Box (Interval (0,
0), Interval (0, 0)), SCM_EOL));
}
- footnote_number_markups.push_back (markup);
- footnote_number_stencils.push_back (s);
+ else
+ {
+ footnote_number_markups.push_back (markup);
+ footnote_number_stencils.push_back (*s);
+ }
counter++;
}
// find the maximum X_AXIS length
Real max_length = -infinity_f;
for (vsize i = 0; i fn_count; i++)
-max_length = max (max_length, footnote_number_stencils[i]-extent
(X_AXIS).length ());
+max_length = max (max_length, footnote_number_stencils[i].extent
(X_AXIS).length ());
/*
translate each stencil such that it attains the correct maximum length
and bundle the
footnotes into a scheme object.
*/
- SCM *tail = numbers;
- SCM *in_text_tail = in_text_numbers;
for (vsize i = 0; i fn_count; i++)
{
- *in_text_tail =
