Re: T1349 - Fix load order for running with Guile V2 (issue 4849054)

[pnorcks]

The load-order issue appears to be fixed, testing with git and guile 1.8
and 2.0.2.  Ignoring whitespace changes, this patch LGTM.

Some more shuffling is needed to make sure we have markup commands
defined where they need to be, but that's beyond the scope of this
patch.

Thanks,
Patrick

https://codereview.appspot.com/4849054/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: oops! I've changed files in the `snippet' directory

[Graham Percival]

On Thu, Aug 18, 2011 at 08:09:29AM +0200, Werner LEMBERG wrote:
> 
> My question: Is this really a problem?  Are such fixes lost if someone
> is running makelsr?

Depends, and yes.  Files are completely rewritten from a makelsr
import.  Whether or not this is a problem depends on the changes
that you've been making.

> Or is someone taking care of such fixes, probably
> transferring them to the LSR?

Certainly nobody is doing that.  Of course, these days nobody is
doing lsr imports either.

> It's really tedious to add fixed files to the `new' directory...

This might be made easier with a script, or at least if we cleared
out the old stuff from that "new" directory.

Unfortunately, nobody is willing to demonstrate that they care
about LSR, so we're floating in limbo.

Cheers,
- Graham

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

oops! I've changed files in the `snippet' directory

[Werner LEMBERG]

Folks,


while doing some doc fixes I've also changed files directly in the
`snippet' directory by accident.  But this is not me alone, others do
the same :-)

My question: Is this really a problem?  Are such fixes lost if someone
is running makelsr?  Or is someone taking care of such fixes, probably
transferring them to the LSR?

It's really tedious to add fixed files to the `new' directory...


Werner

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Check for null pointer

[Dan Eble]

Carl Sorensen  byu.edu> writes:

> Do you have more information about the segfault that you'd be willing to
> share with us?

What I have so far is a backtrace:
  http://lists.gnu.org/archive/html/lilypond-devel/2011-08/msg00494.html
and a large amount of input spread across many files, which is why I chose to
review the lilypond source first.

It may also be of interest that I am generating PartCombineMusic with scheme
functions other than the stock part combiner.
-- 
Dan



___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Uninitialized SCM variables

[Dan Eble]

On 2011-08-17, at 13:03 , Phil Holmes wrote:

> - Original Message - From: "Graham Percival" 
> 
> To: "Carl Sorensen" 
> Cc: "lilypond-devel Development" 
> Sent: Wednesday, August 17, 2011 5:48 PM
> Subject: Re: Uninitialized SCM variables
> 
> 
>> On Wed, Aug 17, 2011 at 05:53:40AM -0600, Carl Sorensen wrote:
>>> \On 8/16/11 10:25 PM, "Dan Eble"  wrote:
>>> 
>>> > Is there a reason that these variables in lily/profile.cc don't need to > 
>>> > be
>>> > initialized?  I don't have experience with guile, but it looks > 
>>> > dangerous.
>>> 
>>> I guess the code in this section relies on the fact that the compiler will
>>> initialize the unitialized value to zero.   Do you believe that is a
>>> problem?
>> 
>> Is there a special rule that compilers will always initalize
>> uninitialized scheme values to zero?  Because I discovered a
>> segfault just yesterday (in a different program) that was because
>> of gcc [1] not initalizing a variable to 0.
>> 
>> [1] or rather, the C standard does not specify that an
>> uninitalized variable should be set to 0, so I do not blame gcc in
>> the least; it was the programmer at fault.
>> 
>> Cheers,
>> - Graham
> 
> In C-style languages, uninitialised variable are uninitialised and therefore 
> have an indeterminant value.  Hence the danger of uninitialised pointers. 
> Some other languages do initialise them to 0 - visual basic is an example. In 
> more modern languages, (c# is one I'm familiar with) the compile fails if a 
> variable is not explicitly initialised.

Backing up… I believe the compiler will initialize the bits in the 
aforementioned variables to zero, but is zero a desirable default for SCM 
variables in general, and these in particular?

It also just sank in that in another thread there was a statement that treating 
a SCM as a boolean is "very wrong".  That would include a number of lines in 
ly_property_lookup_stats and note_property_access that use these variables.
-- 
Dan


___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

PATCH: 48-hour countdown 21:00 MDT 2011-08-19

[Colin Campbell]

For 21:00 MDT Friday August 19

Issue 1349 : 
Guile 2.0 compat: Scheme macros (repeated due to revisions since the 
Monday list)


Issue 1779 : 
accidentaled notes too far from the barline - R Issue 4188051 
: Remove special case in 
staff-spacing


Issue 804 : Code 
cleaning: checking types-conversion issues and other build warnings R


Issue 472 : 
collision rest + (accidental/notehead) with beaming - R Issue 4860043 
: Better pure height 
approximations for beamed rests.


Issue 1785 : 
Compressible space before the first note of a line - R Issue 4188051 
: Remove special case in 
staff-spacing


Issue 509 : 
collision nested tuplet numbers - R Issue 4808082 
  (repeated after revisions)


Issue 1776 : 
Doc: NR - Polymetric Notation \compoundMeter isn't documented - R Issue 
4837050 




Cheers,

Colin


--
The human race has one really effective weapon, and that is laughter.
-- Mark Twain

 

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

DOC: Revise CG 3.4 Commit Access (issue 4898058)

[percival . music . ca]

LGTM, one suggestion.


http://codereview.appspot.com/4898058/diff/1/Documentation/contributor/source-code.itexi
File Documentation/contributor/source-code.itexi (right):

http://codereview.appspot.com/4898058/diff/1/Documentation/contributor/source-code.itexi#newcode1574
Documentation/contributor/source-code.itexi:1574: making commits.
An alternate method would be to put the same RSA private+public key on
every machine.

Since this is aimed at serious developers, let's include both methods.
If it confuses somebody, they can always ask.

http://codereview.appspot.com/4898058/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Likely a good frog project for someone with C knowledge

[Han-Wen Nienhuys]

On Wed, Aug 17, 2011 at 9:41 AM, David Kastrup  wrote:

>> on the plus side, if we use this, we will be the first GNU program to
>> be compatible with the elisp compatibility mode in GUILE that has been
>> almost ready for the last 15 years.
>
> I should say that would be rather irrelevant as a design goal.  In any
> case, the respective define in current master (2.whatever) is

I should probably start using  tags.



-- 
Han-Wen Nienhuys - han...@xs4all.nl - http://www.xs4all.nl/~hanwen

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Fix 1805: AmbitusAccidental needs avoid-slur, needed when the notes in the ambitus have a slur (issue 4904049)

[n . puttock]

http://codereview.appspot.com/4904049/diff/1/scm/define-grobs.scm
File scm/define-grobs.scm (right):

http://codereview.appspot.com/4904049/diff/1/scm/define-grobs.scm#newcode125
scm/define-grobs.scm:125: (avoid-slur . inside)
I think it would make more sense for the engravers to ignore an
AmbitusAccidental by changing the acknowledger to use
inline-accidental-interface.

http://codereview.appspot.com/4904049/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: T1349 - Fix load order for running with Guile V2 (issue 4849054)

[n . puttock]

LGTM.

http://codereview.appspot.com/4849054/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: GOP-PROP 8: issue priorities (probable 2)

[Graham Percival]

On Tue, Aug 16, 2011 at 11:20:02AM +0100, Ian Hulin wrote:
> 1. Some nit-picky stuff to make the proposal crystal-clear to
> skim-readers like me.
> See comments below embedded in the your original message text.

Thanks, all fixed.

> 2. I'd like to consider two types to use as additional info to the
> current ones: Type-User-development and Type-Developer-development.

Interesting idea, but I don't see type-user-development as being
useful.  If somebody wannts a certain behavior -- for whatever
reason -- then IMO that's a plain enhancement request.

I'll leave the door open to discussing this in a future GOP
proposal, though.  But right now I want to get the "remove
priorities" proposal passed without getting derailed.

> On 16/08/11 05:51, Graham Percival wrote:
> > ** Shutting up users
> This is a proposal.  It's a bit formal, so tone down your LilyPond lists
> persona a bit and call it something like
> "**Identifying user priorities"

But I don't care about user priorities... ok, I've changed this to
"reminding users about stars", and rewritten it in with a more
netural one.

> "We will remind users that they can 'star' issues which are important to
> them.  They may or may not be relevant to developing the project, or may
> need to be re-worked in terms of a suggested solution.  Using the star
> system will allow contributors to triage user-supplied issues and then
> tell the bug squad they intend to spend time on it.  The contributor
> then uses the Type-*** label to do this."

I used this instead:
We can remind users that they can @qq{star} an issue to indicate
that they care about it.  Since we resolved to treat developers as
independent volunteers, there is no expectation that anybody will
look at those stars, but if any developer want to organize their
work schedule according to the stars, they are welcome to do so.

I like the reminder about GOP-PROP 7.  (well, not an explicit
reminder, but it's still there in the "independent volunteers"
bit)


New version uploaded, and the final one will be tomorrow.

Cheers,
- Graham

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: New engraver for braces (issue 4807053)

[mtsolo]

Most of my comments below resemble each other, but up here I want to
suggest that if you go down the road of generalizing the arpeggio grob,
the places you'd have to do work are in the rhythmic column engraver and
in the several NoteColumn functions that look for an arpeggio.  Instead,
this would be generalized to LeftSideIndication and maybe a
LeftSideIndicationSpanner to coordinate the horizontal deployment.

Great work in making your way through a lot of Lily code to get this up
and running!  I get the sense that you have a good understanding of the
code base, which will serve you well for future endeavors.

Cheers,
MS


http://codereview.appspot.com/4807053/diff/18001/lily/brace-engraver.cc
File lily/brace-engraver.cc (right):

http://codereview.appspot.com/4807053/diff/18001/lily/brace-engraver.cc#newcode1
lily/brace-engraver.cc:1: /*
Check out my comment for Span_brace_engraver.

Of all the engravers in LilyPond, I think that the
new-fingering-engraver is the one that can act as the best model for the
creation of new engravers insofar as it handles many different grobs
with the same method.  I think that this can be rolled into the arpeggio
engraver with a few extra functions.

http://codereview.appspot.com/4807053/diff/18001/lily/span-brace-engraver.cc
File lily/span-brace-engraver.cc (right):

http://codereview.appspot.com/4807053/diff/18001/lily/span-brace-engraver.cc#newcode1
lily/span-brace-engraver.cc:1: /*
Is there any way to combine this with the span arpeggio engraver?
It seems like stop_translation_timestep could be moved to another
function (ie process_spanning_grob) and then could handle multiple grobs
(span_brace_, span_arpeggio_, etc.).

http://codereview.appspot.com/4807053/diff/18001/lily/span-brace-engraver.cc#newcode6
lily/span-brace-engraver.cc:6:
I don't really remember having anything to do with this file...was I
involved in this?  I'm getting old if I can't remember this sorta thing!

If you're comfortable adding your e-mail address, that helps people who
want to edit the file later.

http://codereview.appspot.com/4807053/diff/18001/scm/define-grobs.scm
File scm/define-grobs.scm (right):

http://codereview.appspot.com/4807053/diff/18001/scm/define-grobs.scm#newcode501
scm/define-grobs.scm:501:
Just food for thought: currently, the "Script" grob encompasses many
things (staccati, tenuti, etc.), all of which could be their own grobs,
but are common and multiple enough to be grouped under one grob.  In
general, I would work with the rule of "one, two, and many."  That is,
if you see something being one grob, then leave it as one.  If there are
two (or max(ish) three) that are similar (like the piano pedal stuff) it
is ok to have multiple grobs.  But once you cross the threshold into
many, it's better to handle them like scripts are handled.  I agree that
braces are currently too-coupled with arpeggio's functionality, but
before they become a separate grob, I'd encourage you to think about how
plausible it is that other grobs like this one will need to be created.
If you can envision many, then roll this into arpeggio (or even change
the name of arpeggio to SideSpanner to be more neutral about the actual
content), allow for multiple ones attached to one note column, have some
sorta interface/uber-grob for determining horizontal order, and go down
that route.

http://codereview.appspot.com/4807053/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

GOP-PROP 9: behavior of make doc (probable decision)

[Graham Percival]

Not much discussion, not much change from the last version.

http://lilypond.org/~graham/gop/gop_9.html


Proposal summary

If there are build problems, then it should be easier to find out
why it’s failing. This will be achieved with log files, as well as
possibly including scripts which automatically display portions of
those log files for a failing build.

We will also add targets for building a specific manual (for
quick+easy checking of doc work), as well as for building all
documentation in a specific language (either English or a
translated language).
Rationale

When the lilypond doc build breaks, it’s too hard to figure out
why it broke.

We see emails to lilypond-devel approximately once every four
months about broken doc builds. On a subjective note, Graham has
been the documentation editor since 2003, but even he cannot
reliably pinpoint the cause of a failing doc build within 10
minutes. We waste a ridiculous amount of time, effort, and
patience on doc build problems.
Sea of output

Before any of the current work on reducing output from make, the
result of a “make doc” was over 500,000 lines of text. The prime
reason for the output being so verbose is that all the processes
that run as a result of the call to make echo their output to the
screen, often in verbose mode. Lilypond itself produces around
370,000 lines of output as a result of lilypond-book building all
the snippets.

Much of this output can be redirected to logfiles and so the
impossible-to-read clutter on the screen is cut out and could be
referred to later.
Proposal details

When you run make doc,

* All output will be saved to various log files, with the
* exception of output directly from make(1).

  Note that make(1) refers to a specific executable file on
unix computers, and is not a general term for the build system.
* By default, no other output will be displayed on the
* console, with one exception: if a build fails, we might
* display some portion(s) of log file(s) which give useful
* clues about the reason for the failure.

  The user may optionally request additional output to be
printed; this is controlled with the VERBOSE=x flag. In such
cases, all output will still be written to log files; the console
output is strictly additional to the log files.
* Logfiles from calling lilypond (as part of lilypond-book)
* will go in the relevant
* ‘build/out/lybook-db/12/lily-123456.log’ file. All other
* logfiles will go in the ‘build/logfiles/’ directory.

  A single make doc will therefore result in hundreds of log
files. Log files produced from individual lilypond runs are not
under our control; apart from that, I anticipate having one or two
dozen log files. As long as it is clear which log file is
associated with which operation(s), I think this is entirely
appropriate. The precise implementation will be discussed for
specific patches as they appear.
* Both stderr and stdout will be saved in *.log. The order of
* lines from these streams should be preserved.
* There will be no additional “progress messages” during the
* build process. If you run make --silent, a non-failing build
* should print absolutely nothing to the screen.
* Assuming that the loglevels patch is accepted, lilypond
* (inside lilypond-book) will be run with –loglevel=WARN.
* http://codereview.appspot.com/4822055/
* Ideally, a failing build should provide hints about the
* reason why it failed, or at least hints about which log
* file(s) to examine. 

If this proposal is accepted, none of these policies will be
assumed to apply to any other aspect of the build system. Policies
for any other aspect of the build system will be discussed in
separate proposals.
Don’t cause more build problems

However, there is a danger in this approach, that vital error
messages can also be lost, thus preventing the cause of the
failure of a make being found. We therefore need to be
exceptionally careful to move cautiously, include plenty of tests,
and give time for people to experiment/find problems in each stage
before proceeding to the next stage.

This will be done by starting from individual lilypond calls
within lilypond-book, and slowly moving to “larger” targets of the
build system – after the individual lilypond calls are are
producing the appropriate amount of output and this is saved in
the right place and we can automatically isolate parts of a
failing build, we will work on lilypond-book in general, and only
then will we look at the build system itself.
Implementation notes

There is an existing make variable QUIET_BUILD, which alter the
amount of output being displayed
(http://lilypond.org/doc/v2.15/Documentation/contributor/useful-make-variables
). We are not planning on keeping this make variable.

The standard way for GNU packages to give more output is with a
V=x option. Presumably this is done by increasing x? If we support
this option, we should still write log 

Re: Fixes issue 1628. (issue 4876051)

[k-ohara5a5a]

LGTM

Now string numbers move around slurs as well.
That didn't work in the old patch, nor in 2.14.

The old regtest string-number-around-slur.ly avoided collisions only by
accident.  Different pitches would cause collisions with the string
numbers, but after this patch they really move #'around the slur.


http://codereview.appspot.com/4876051/diff/8001/input/regression/string-number-around-slur.ly
File input/regression/string-number-around-slur.ly (right):

http://codereview.appspot.com/4876051/diff/8001/input/regression/string-number-around-slur.ly#newcode9
input/regression/string-number-around-slur.ly:9: \textLengthOn
Alternatively, if you remove \textLengthOn, the numbers go to their
original in-in-out positions, and the natural spacing gives a more
realistic test.

http://codereview.appspot.com/4876051/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Doc: NR Warning added to para for cueduring (issue 4850051)

[k-ohara5a5a]

LGTM.
Recently the same issue came up with \quoteDuring

(that bug report has other issues as well)

That is, starting a Staff with a quote prints nothing.
  \addQuote "A" {c' d' e' f' c' d' e' f'}
  \new Staff{ \quoteDuring "A" s1  r1}
Could you add a parallel warning to the \quoteDuring section?


http://codereview.appspot.com/4850051/diff/3001/Documentation/notation/staff.itely
File Documentation/notation/staff.itely (right):

http://codereview.appspot.com/4850051/diff/3001/Documentation/notation/staff.itely#newcode1181
Documentation/notation/staff.itely:1181:
Same issue as with \cueDuring, happens with \quoteDuring

When a @code{Voice} starts with @code{\quoteDuring} the @code{Voice}
context must be explicitly declared.

http://codereview.appspot.com/4850051/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: cartouche collides with heading

[Werner LEMBERG]

>> This looks like a bug.  Could you please report it to bug-texinfo
>> (together with a confirmation that the cartouche problem has been
>> solved)?
> 
> It looks like I'd need to subscribe to another mailing list to do
> that.  Is this true, or can input be made without being subscribed?

You can submit bug reports without being subscribed; the list
administrator (also Karl Berry) eventually forwards your mail
accordingly after some days.

> If I do need to subscribe, could I pretty please ask you to report
> it and confirm the other bug fixed?  I have a tiny test case that
> can be submitted, plus details of a revision level that did work and
> one that doesn't, which I can supply to you ready-to-go.

I can also do a bug report if you send me a small test file.


Werner

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

New short and long lyric ties. (issue 4912041)

[bordage . bertrand]

Reviewers: ,

Message:
Hi everyone,

This follows 8d148ea05fa4b34f8cc3407e112363d715b27ad8

This is fully working, except for a small issue in make doc.
The two examples I put in the doc are working alone, but not with make
doc:
there should be short ties in "~è~", but we mysteriously get medium
ties.
This works if we change "è" for an ASCII character.
I think this is somehow due to an encoding issue in the "make doc"
process.

Cheers,
Bertrand

Description:
New short and long lyric ties.

Please review this at http://codereview.appspot.com/4912041/

Affected files:
  M Documentation/de/notation/vocal.itely
  M Documentation/es/notation/vocal.itely
  M Documentation/fr/notation/vocal.itely
  M Documentation/notation/vocal.itely
  M mf/feta-ties.mf
  M scm/define-markup-commands.scm


___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Uninitialized SCM variables

[Graham Percival]

On Wed, Aug 17, 2011 at 07:26:19PM +0200, David Kastrup wrote:
> Graham Percival  writes:
> 
> > [1] or rather, the C standard does not specify that an uninitalized
> > variable should be set to 0, so I do not blame gcc in the least; it
> > was the programmer at fault.
> 
> The C standard guarantees binary zeros for statically allocated
> uninitialized variables.

Ok, in my case it was an uninitalized member variable, on a G5
machine, with something like gcc 4.01 ?  It was dying of memory
when trying to allocated an array of 96x10497652 doubles, because
the programmer forgot to initialize the second variable to 2.

I really dislike languages that allow uninitalized variables; I
dislike dealing with such problems so much that I'm a fan of the
functional "bind a value to a variable and never change it"
approach.  (seen in Mozart/Oz, and probably other languages as
well)  let the compiler optimize for memory reuse and stuff!

Cheers,
- Graham

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Uninitialized SCM variables

[David Kastrup]

"Phil Holmes"  writes:

> In C-style languages, uninitialised variable are uninitialised and
> therefore have an indeterminant value.

Wrong for statically allocated variables.

> Hence the danger of uninitialised pointers. Some other languages do
> initialise them to 0 - visual basic is an example. In more modern
> languages, (c# is one I'm familiar with) the compile fails if a
> variable is not explicitly initialised.

In C++, the compilation is not guaranteed to succeed if the variable is
not explicitly _instantiated_ in some compilation unit (rather than just
being declared as extern).  An initialization need not happen: binary
zeros is the default.  Unless we are talking classes with constructors.

-- 
David Kastrup


___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Uninitialized SCM variables

[David Kastrup]

Graham Percival  writes:

> On Wed, Aug 17, 2011 at 05:53:40AM -0600, Carl Sorensen wrote:
>> \On 8/16/11 10:25 PM, "Dan Eble"  wrote:
>> 
>> > Is there a reason that these variables in lily/profile.cc don't need to be
>> > initialized?  I don't have experience with guile, but it looks dangerous.
>> 
>> I guess the code in this section relies on the fact that the compiler will
>> initialize the unitialized value to zero.   Do you believe that is a
>> problem?
>
> Is there a special rule that compilers will always initalize
> uninitialized scheme values to zero?  Because I discovered a
> segfault just yesterday (in a different program) that was because
> of gcc [1] not initalizing a variable to 0.

The C runtime initializes static storage to binary zeros.  That is
guaranteed.  For automatic variables, all guesses are off.  The typical
multiuser operating system will initialize stack areas to zeros when
they get mapped the first time (uninitialized memory could leak
information), but only when the area is used the first time.  After
that, the values depend on the history of previous function calls.

> [1] or rather, the C standard does not specify that an uninitalized
> variable should be set to 0, so I do not blame gcc in the least; it
> was the programmer at fault.

The C standard guarantees binary zeros for statically allocated
uninitialized variables.

-- 
David Kastrup


___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Uninitialized SCM variables

[Phil Holmes]

- Original Message - 
From: "Graham Percival" 

To: "Carl Sorensen" 
Cc: "lilypond-devel Development" 
Sent: Wednesday, August 17, 2011 5:48 PM
Subject: Re: Uninitialized SCM variables



On Wed, Aug 17, 2011 at 05:53:40AM -0600, Carl Sorensen wrote:

\On 8/16/11 10:25 PM, "Dan Eble"  wrote:

> Is there a reason that these variables in lily/profile.cc don't need to 
> be
> initialized?  I don't have experience with guile, but it looks 
> dangerous.


I guess the code in this section relies on the fact that the compiler 
will

initialize the unitialized value to zero.   Do you believe that is a
problem?


Is there a special rule that compilers will always initalize
uninitialized scheme values to zero?  Because I discovered a
segfault just yesterday (in a different program) that was because
of gcc [1] not initalizing a variable to 0.

[1] or rather, the C standard does not specify that an
uninitalized variable should be set to 0, so I do not blame gcc in
the least; it was the programmer at fault.

Cheers,
- Graham


In C-style languages, uninitialised variable are uninitialised and therefore 
have an indeterminant value.  Hence the danger of uninitialised pointers. 
Some other languages do initialise them to 0 - visual basic is an example. 
In more modern languages, (c# is one I'm familiar with) the compile fails if 
a variable is not explicitly initialised.



--
Phil Holmes



___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Uninitialized SCM variables

[Graham Percival]

On Wed, Aug 17, 2011 at 05:53:40AM -0600, Carl Sorensen wrote:
> \On 8/16/11 10:25 PM, "Dan Eble"  wrote:
> 
> > Is there a reason that these variables in lily/profile.cc don't need to be
> > initialized?  I don't have experience with guile, but it looks dangerous.
> 
> I guess the code in this section relies on the fact that the compiler will
> initialize the unitialized value to zero.   Do you believe that is a
> problem?

Is there a special rule that compilers will always initalize
uninitialized scheme values to zero?  Because I discovered a
segfault just yesterday (in a different program) that was because
of gcc [1] not initalizing a variable to 0.

[1] or rather, the C standard does not specify that an
uninitalized variable should be set to 0, so I do not blame gcc in
the least; it was the programmer at fault.

Cheers,
- Graham

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: New engraver for braces (issue 4807053)

[bordage . bertrand]

That wasn't working 'cause I forgot to add some files to git...
This is now fixed.

Bertrand

http://codereview.appspot.com/4807053/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Likely a good frog project for someone with C knowledge

[David Kastrup]

Han-Wen Nienhuys  writes:

> On Wed, Aug 17, 2011 at 5:19 AM, David Kastrup  wrote:
>
>> That would be scm_is_null (x).  It is not exactly like the code gets
>> less readable by that substitution.
>
> it's not the same though. scm_is_null expands to
>
> pairs.h:#define scm_is_null(x)(scm_is_null_or_nil(x))

That would be a newer development, I guess.  In the 1.8 sources this
just compares to SCM_EOL.

> on the plus side, if we use this, we will be the first GNU program to
> be compatible with the elisp compatibility mode in GUILE that has been
> almost ready for the last 15 years.

I should say that would be rather irrelevant as a design goal.  In any
case, the respective define in current master (2.whatever) is

/*
 * See the comments preceeding the definitions of SCM_BOOL_F and
 * SCM_MATCHES_BITS_IN_COMMON in tags.h for more information on
 * how the following macro works.
 */
#define scm_is_null_or_nil(x)  \
  (SCM_MATCHES_BITS_IN_COMMON ((x), SCM_ELISP_NIL, SCM_EOL))

Now that's presumably hardly less efficient, in particular not requiring
an additional branch.  I have a hard time imagining why it would make
sense to let Lisp compatibility invade the API like that, but short of
making the assembly code look somewhat unexpected, it would not likely
affect performance.

-- 
David Kastrup

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Likely a good frog project for someone with C knowledge

[Ian Hulin]

On 17/08/11 07:41, Graham Percival wrote:
> On Tue, Aug 16, 2011 at 11:14:35PM -0300, Han-Wen Nienhuys wrote:
>> On Tue, Aug 16, 2011 at 7:17 PM, David Kastrup 
>> wrote:
>>> So what does relying on undefined behavior buy us apart from
>>> the inability to debug type errors?
>> 
>> It buys us time to work on more interesting and more valuable
>> improvements.
> 
> By amazing coincidence, we have a potential Frog (I heard about her
> about 24 hours ago) for whom this task is absolutely ideal. She has
> experience with lilypond, scheme, C++, and assorted other 
> languages.  She even has a mentor!
> 
> I'm not suggesting that a major developer take the time to clean up
> these things, but this seems like a fantastic way for her to "get
> her feet wet" with the scheme/macro/C++ stuff -- and my vague 
> impression is that this is one area that contributors find the most
> confusing.  I've heard both scheme people and C++ people complain
> that they can work on either scheme or C++, but as soon as a bug
> touches both areas they give up.  If she works on this, asks
> questions about stuff she doesn't understand, and we add the 
> clarifications to the CG, I think this will be a great initial Frog
> project.
> 
> Cheers, - Graham
1+

Cheers Ian


___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Likely a good frog project for someone with C knowledge

[Han-Wen Nienhuys]

On Wed, Aug 17, 2011 at 5:19 AM, David Kastrup  wrote:

> --- a/lily/general-scheme.cc
> +++ b/lily/general-scheme.cc
> @@ -381,7 +381,7 @@ LY_DEFINE (ly_stderr_redirect, "ly:stderr-redirect",
>   string m = "w";
>   string f = ly_scm2string (file_name);
>   FILE *stderrfile;
> -  if (mode != SCM_UNDEFINED && scm_string_p (mode))
> +  if (scm_is_string (mode))
>
> Why would anybody write a condition like
>  if (mode != SCM_UNDEFINED && scm_string_p (mode))
> if he did not first try just scm_string_p (mode), could not figure out
> why it didn't work, and then added a check against SCM_UNDEFINED?

SCM_UNDEFINED is used to indicate a missing optional argument.  If you
look at general-scheme.cc that should be pretty obvious, because
pretty much every function looks like that.

I have spent 10 minutes looking in vain through the documentation to
find the "official" solution for optional arguments in the C
interface, but haven't been able to find it. In guile there are a
bunch of macros in validate.h, but they do not appear in the
documentation.

Oh, I just found it now.

http://www.gnu.org/software/guile/manual/html_node/API-Overview.html#API-Overview

"For some Scheme functions, some last arguments are optional; the
corresponding C function must always be invoked with all optional
arguments specified. To get the effect as if an argument has not been
specified, pass SCM_UNDEFINED as its value. You can not do this for an
argument in the middle; when one argument is SCM_UNDEFINED all the
ones following it must be SCM_UNDEFINED as well."

I guess we could use

 SCM_UNBNDP()

instead.  One thing I dislike about converting to the 'official' API
is that it will litter our source-code with equally inscrutable
macros.  Perhaps we should just roll our own?

  inline SCM ly_is_unbound(SCM)

looks much nicer to me.

>> Speaking as a long-time lilypond developer, it is my experience that
>> the errors you point out are not a problem (except for the SCM => bool
>> conversion).  GUILE's API uses data that can be passed into C
>> functions efficiently as parameters.  This means that the SCM type
>> must be a machine word, so the genericity suggested by the GUILE docs
>> are a joke.
>
> Except that there are insiders and outsiders to the joke.  Do you really
> consider yourself infallible so that you refuse to write code that would
> be recognizably correct for someone doing a code review?
>
> Code reviews are not fun.  And exactly because you want to buy yourself
> time to work on more interesting and more valuable improvements, you
> should strive to write code that can be reviewed by lesser people.
> Those that actually read the APIs of Guile.  Or in this case, write code
> that can be reviewed for correctness by the compiler.  The compiler is
> not overly eager to write more interesting and more valuable
> improvements, but it is a rather thorough and experienced reviewer.
>
>> If you feel compelled to change large swaths of source code by
>> substituting x == SCM_EOL with scm_is_eq(x, SCM_EOL), then I can't
>> stop you, but to me it just looks like a waste of time.
>
> That would be scm_is_null (x).  It is not exactly like the code gets
> less readable by that substitution.

it's not the same though. scm_is_null expands to

pairs.h:#define scm_is_null(x)  (scm_is_null_or_nil(x))

on the plus side, if we use this, we will be the first GNU program to
be compatible with the elisp compatibility mode in GUILE that has been
almost ready for the last 15 years.

--
Han-Wen Nienhuys - han...@xs4all.nl - http://www.xs4all.nl/~hanwen

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Uninitialized SCM variables

[David Kastrup]

Carl Sorensen  writes:

> \On 8/16/11 10:25 PM, "Dan Eble"  wrote:
>
>> Is there a reason that these variables in lily/profile.cc don't need to be
>> initialized?  I don't have experience with guile, but it looks dangerous.
>> 
>> SCM context_property_lookup_table;
>> SCM grob_property_lookup_table;
>> SCM prob_property_lookup_table;
>
> I guess the code in this section relies on the fact that the compiler will
> initialize the unitialized value to zero.   Do you believe that is a
> problem?

0 is not a value that the garbage collector will see meaning in.

A detailed analysis will likely find that this causes no problems, but
where is the point in keeping code around that requires a detailed
analysis?  It would be better to spend the analysis on real
complications rather than sloppy code.

-- 
David Kastrup


___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Uninitialized SCM variables

[Reinhold Kainhofer]

Am Wednesday, 17. August 2011, 13:53:40 schrieb Carl Sorensen:
> \On 8/16/11 10:25 PM, "Dan Eble"  wrote:
> > Is there a reason that these variables in lily/profile.cc don't need to
> > be initialized?  I don't have experience with guile, but it looks
> > dangerous.
> > 
> > SCM context_property_lookup_table;
> > SCM grob_property_lookup_table;
> > SCM prob_property_lookup_table;
> 
> I guess the code in this section relies on the fact that the compiler will
> initialize the unitialized value to zero.   Do you believe that is a
> problem?

Note that this is also the assumption on the !cached, even if changed to use 
SCM_UNPACK:

SCM cached;
if (!SCM_UNPACK (cached)) ...

where SCM_UNPACK is defined in /usr/include/libguile/tags.h to be either
   #define SCM_UNPACK(x) (x)
or
   #define SCM_UNPACK(x) ((x).n.n)

In both cases, the check is whether an uninitialized variable is 0...

Cheers,
Reinhold

-- 
--
Reinhold Kainhofer, reinh...@kainhofer.com, http://reinhold.kainhofer.com/
 * Financial & Actuarial Math., Vienna Univ. of Technology, Austria
 * http://www.fam.tuwien.ac.at/, DVR: 0005886
 * LilyPond, Music typesetting, http://www.lilypond.org

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Check for null pointer

[Carl Sorensen]

On 8/16/11 11:15 PM, "Dan Eble"  wrote:

> I think the following code should check that ev is not null before
> dereferencing it.  This might be a factor in a segfault I saw.  (Sorry to be
> so vague, but I am not going to build lilypond to test the theory.  It would
> take a credible threat of violence to change my mind.)  A grep suggests that
> there may be other similar cases.
> 
> void
> Dispatcher::dispatch (SCM sev)
> {
>   Stream_event *ev = unsmob_stream_event (sev);
>   SCM class_symbol = ev->get_property ("class");

Certainly there would be a segfault if sev is null.  And perhaps this is the
best place to check for it.  But this would not be the best place to place a
warning and handle the null sev, because there is no context for how the
null sev got in the call.

Do you have more information about the segfault that you'd be willing to
share with us?

Thanks,

Carl


___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: And another frog task

[Reinhold Kainhofer]

Am Wednesday, 17. August 2011, 13:42:50 schrieb Carl Sorensen:
> On 8/17/11 5:35 AM, "David Kastrup"  wrote:
> > So it would seem like another worthwhile frog task to get rid of
> > SCM_CDRLOC in the source tree when feasible (when the list is being
> > consulted front-to-back while it is in the process of being created,
> > this would not be possible.  However, since tail pointers in this manner
> > can't be kept around beyond the function call, chances are that we have
> > very few such cases if at all).
> > 
> > It will simplify the code, make it more readable and faster, and
> > decrease the amount of stack scanning that can go wrong.
> 
> And there are only 29 occurrences, so it's a smaller task the the task of
> rationalizing the guile interface.

It's smaller, but it requires a deeper analysis of the code (where the list 
might be used meanwhile during the loop, whether the list might already 
contain elements,e tc.).
Most of the guile interface issues are simply copy-and-paste of very few 
templates,e.g. 
  -) "something == SCM_EOL"=>"scm_is_null (something)"
  -) "something == SCM_BOOL_T"=>"scm_is_true (something)"
  -) "scm_equal_p (a, b_) == SCM_BOOL_F"=>"!ly_is_equal (a, b)"

Cheers,
Reinhold
-- 
--
Reinhold Kainhofer, reinh...@kainhofer.com, http://reinhold.kainhofer.com/
 * Financial & Actuarial Math., Vienna Univ. of Technology, Austria
 * http://www.fam.tuwien.ac.at/, DVR: 0005886
 * LilyPond, Music typesetting, http://www.lilypond.org

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Uninitialized SCM variables

[Carl Sorensen]

\On 8/16/11 10:25 PM, "Dan Eble"  wrote:

> Is there a reason that these variables in lily/profile.cc don't need to be
> initialized?  I don't have experience with guile, but it looks dangerous.
> 
> SCM context_property_lookup_table;
> SCM grob_property_lookup_table;
> SCM prob_property_lookup_table;

I guess the code in this section relies on the fact that the compiler will
initialize the unitialized value to zero.   Do you believe that is a
problem?

Thanks,

Carl


___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: And another frog task

[Carl Sorensen]

On 8/17/11 5:35 AM, "David Kastrup"  wrote:


> 
> So it would seem like another worthwhile frog task to get rid of
> SCM_CDRLOC in the source tree when feasible (when the list is being
> consulted front-to-back while it is in the process of being created,
> this would not be possible.  However, since tail pointers in this manner
> can't be kept around beyond the function call, chances are that we have
> very few such cases if at all).
> 
> It will simplify the code, make it more readable and faster, and
> decrease the amount of stack scanning that can go wrong.

And there are only 29 occurrences, so it's a smaller task the the task of
rationalizing the guile interface.

Thanks,

Carl


___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

And another frog task

[David Kastrup]

I just profiled the following code:

#include 
#include 

void *test1(void *n)
{
  for (int i=0; i<(int)n; i++) {
SCM list = SCM_EOL;
SCM *tail = &list;
for (int k=0; k<(int)n; k++) {
  *tail = scm_cons (SCM_BOOL_F, SCM_EOL);
  tail = SCM_CDRLOC(*tail);
}
  }
  return 0;
}

void* test2(void *n)
{
  for (int i=0; i<(int)n; i++) {
SCM list = SCM_EOL;
for (int k=0; k<(int)n; k++) {
  list = scm_cons (SCM_BOOL_F, list);
}
list = scm_reverse_x (list, SCM_EOL);
  }
  return 0;
}

  
main(int ac, char**av)
{
  int n = atoi(av[1]);
  if (n<0)
scm_with_guile (test1, (void*)-n);
  else
scm_with_guile (test2, (void*)n);
  return 0;
}


I profiled both functions in separate runs to make sure the differences
were not because of hot/cold cache or heap.  The results were that
consing the list together from front to end (test1, corresponding with
the Lilypond manner of creating lists) were about 20% slower without,
40% slower with optimization in contrast to consing it together in
reverse order, then reversing.

Variant 1 "looks" more efficient and is what Lilypond does when it
creates linear lists in order.  It is, however, considerably less
efficient, _and_ it needs additional variables, _and_ it is quite more
complex to understand.  In addition, the tail pointer is opaque to the
conservative garbage collector, so you need to make sure that you do
things in the right order so that the tail does not fall victim to the
garbage collector.

So it would seem like another worthwhile frog task to get rid of
SCM_CDRLOC in the source tree when feasible (when the list is being
consulted front-to-back while it is in the process of being created,
this would not be possible.  However, since tail pointers in this manner
can't be kept around beyond the function call, chances are that we have
very few such cases if at all).

It will simplify the code, make it more readable and faster, and
decrease the amount of stack scanning that can go wrong.

-- 
David Kastrup


___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Fixes issue 1628. (issue 4876051)

[mtsolo]

New patchset uploaded w/ minimal changes made (just the engravers).

http://codereview.appspot.com/4876051/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Fix for Issue 620. (issue 4814041)

[mtsolo]

Pushed as ac7aef03ab9d459a6ea6f03d9c127be150871dd4.

Cheers,
MS

http://codereview.appspot.com/4814041/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Prevents nested tuplets from colliding. (issue 4808082)

[mtsolo]

I haven't gotten around to making the other changes you pointed out

yet, but I

will either tonight or tomorrow.



Cheers,
MS


Changes are up and ready for review.

Cheers,
MS


http://codereview.appspot.com/4808082/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Likely a good frog project for someone with C knowledge

[David Kastrup]

Reinhold Kainhofer  writes:

> On Mi., 17. Aug. 2011 10:19:33 CEST, David Kastrup  wrote:
>> I mean, look at the bad code I dug up.    Pretty early in the list there
>> was:
>> -    if (mode != SCM_UNDEFINED && scm_string_p (mode))
>> +    if (scm_is_string (mode))
>
>
> Yes, that's code that should really be fixed.

The "+" is the indicator of a git diff.  I already pushed the results of
my cursory grep pattern code review.

-- 
David Kastrup


___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Likely a good frog project for someone with C knowledge

[David Kastrup]

Reinhold Kainhofer  writes:

> On Mi., 17. Aug. 2011 10:19:33 CEST, David Kastrup  wrote:
>> I mean, look at the bad code I dug up.    Pretty early in the list there
>> was:
>> -    if (mode != SCM_UNDEFINED && scm_string_p (mode))
>> +    if (scm_is_string (mode))
>
>
> Yes, that's code that should really be fixed.
>
>
>> > If you feel compelled to change large swaths of source code by
>> > substituting x == SCM_EOL with scm_is_eq(x, SCM_EOL), then I can't
>> > stop you, but to me it just looks like a waste of time.
>> 
>> That would be scm_is_null (x).    It is not exactly like the code gets
>> less readable by that substitution.
>
> Here, I agree with David, too.
> If we have someone who wants to work on them and clean up some code, I
> have no objection. It just probably won't fix a problem, but improves
> readability and code style.
>
> The only proble I see with the -D compile switch is the code of
> ly_symbol2scm (which is used several times in almost every file),
> which does a check "if (!cached)" to see if the SCM cache has been
> initialized. How should this be correctly implemented? It is not a
> check for a scheme value, but builds on the guile internals of how a
> SCM looks when initialized.

I already wrote that.

if (!SCM_UNPACK (cached))

We already use SCM_UNPACK to access raw content when creating smobs (and
it is both the only and the canonical way to do this).

-- 
David Kastrup

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Likely a good frog project for someone with C knowledge

[Reinhold Kainhofer]

On Mi., 17. Aug. 2011 10:19:33 CEST, David Kastrup  wrote:
> I mean, look at the bad code I dug up.    Pretty early in the list there
> was:
> -    if (mode != SCM_UNDEFINED && scm_string_p (mode))
> +    if (scm_is_string (mode))


Yes, that's code that should really be fixed.


> > If you feel compelled to change large swaths of source code by
> > substituting x == SCM_EOL with scm_is_eq(x, SCM_EOL), then I can't
> > stop you, but to me it just looks like a waste of time.
> 
> That would be scm_is_null (x).    It is not exactly like the code gets
> less readable by that substitution.

Here, I agree with David, too.
If we have someone who wants to work on them and clean up some code, I have no 
objection. It just probably won't fix a problem, but improves readability and 
code style.

The only proble I see with the -D compile switch is the code of ly_symbol2scm 
(which is used several times in almost every file), which does a check "if 
(!cached)" to see if the SCM cache has been initialized. How should this be 
correctly implemented? It is not a check for a scheme value, but builds on the 
guile internals of how a SCM looks when initialized.

BTW, it might be useful to compile with -fno-inline to get warnings from 
inlined functions directly rather than at the spot where they are called.

Cheers,
Reinhold

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Likely a good frog project for someone with C knowledge

[David Kastrup]

David Kastrup  writes:

> Bertrand Bordage  writes:
>
>> 2011/8/17 David Kastrup 
>>
>> Bertrand Bordage  writes:
>> 
>> 
>> > This would be great if Han-Wen decides to keep it like that.
>> > Otherwise there is really a lot of work, with many shortcuts to
>> > define.
>> > to_boolean (scm_is_pair (x))
>> 
>> 
>> That one would be wrong since scm_is_pair already returns a C
>> boolean.
>> 
>>
>> I was thinking about scm_pair_p ()...
>> This is typically the kind of mistakes that should be solved.
>> Shame on me, I won't do it again.
>>
>> Where did you find SCM_CONSP ?
>> I don't see it in guile's reference. Maybe something to add to the CG
>> ?
>
> Guile reference manual for Guile 1.8, "Pair data".  It is even indexed.
> What version of the Guile manual are you using?  To quote:
>
>Guile implements pairs by mapping the CAR and CDR of a pair directly
> into the two words of the cell.
>
>  -- Macro: int SCM_CONSP (SCM X)
>  Return non-zero iff X is a Scheme pair object.
>
>  -- Macro: int SCM_NCONSP (SCM X)
>  The complement of SCM_CONSP.

However, I found that as a reasonably dependable heuristic, "functions"
scm_is_* are implemented as macros after all.  That is probably why we
have scm_is_eq, but not scm_is_equal which needs to be done by something
like scm_is_true (scm_equal_p (...)) instead.  Or by ly_is_equal (which
we have).

So here I would just use scm_is_pair and !scm_is_pair.

-- 
David Kastrup


___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Likely a good frog project for someone with C knowledge

[David Kastrup]

Bertrand Bordage  writes:

> 2011/8/17 David Kastrup 
>
> Bertrand Bordage  writes:
> 
> 
> > This would be great if Han-Wen decides to keep it like that.
> > Otherwise there is really a lot of work, with many shortcuts to
> > define.
> > to_boolean (scm_is_pair (x))
> 
> 
> That one would be wrong since scm_is_pair already returns a C
> boolean.
> 
>
> I was thinking about scm_pair_p ()...
> This is typically the kind of mistakes that should be solved.
> Shame on me, I won't do it again.
>
> Where did you find SCM_CONSP ?
> I don't see it in guile's reference. Maybe something to add to the CG
> ?

Guile reference manual for Guile 1.8, "Pair data".  It is even indexed.
What version of the Guile manual are you using?  To quote:

   Guile implements pairs by mapping the CAR and CDR of a pair directly
into the two words of the cell.

 -- Macro: int SCM_CONSP (SCM X)
 Return non-zero iff X is a Scheme pair object.

 -- Macro: int SCM_NCONSP (SCM X)
 The complement of SCM_CONSP.

-- 
David Kastrup

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Likely a good frog project for someone with C knowledge

[David Kastrup]

Han-Wen Nienhuys  writes:

> On Tue, Aug 16, 2011 at 7:17 PM, David Kastrup  wrote:
>>> (and I am speaking as a GUILE developer here as well)
>>
>> So what does relying on undefined behavior buy us apart from the
>> inability to debug type errors?
>
> It buys us time to work on more interesting and more valuable
> improvements.

More time than you want.  And it drives away prospective contributors
who try to get a hang of Lilypond and Guile, read the code and the
respective docs, and find that they need to understand all the internals
of Guile rather than its documented API before being able to understand
what code might and might not be in error, and what code might or might
not make the grownups laugh at them because it looks way more carefully
written than the rest.

I mean, look at the bad code I dug up.  Pretty early in the list there was:

--- a/lily/general-scheme.cc
+++ b/lily/general-scheme.cc
@@ -381,7 +381,7 @@ LY_DEFINE (ly_stderr_redirect, "ly:stderr-redirect",
   string m = "w";
   string f = ly_scm2string (file_name);
   FILE *stderrfile;
-  if (mode != SCM_UNDEFINED && scm_string_p (mode))
+  if (scm_is_string (mode))

Why would anybody write a condition like
  if (mode != SCM_UNDEFINED && scm_string_p (mode))
if he did not first try just scm_string_p (mode), could not figure out
why it didn't work, and then added a check against SCM_UNDEFINED?

This line was committed in 2005 by Jan in its original state.  Why would
Jan add a check against SCM_UNDEFINED before checking for a string?
Most likely because he tried it out and was surprised that an undefined
value passed for a string, and then fixed this curiosity by explicitly
checking for undefined.

> Speaking as a long-time lilypond developer, it is my experience that
> the errors you point out are not a problem (except for the SCM => bool
> conversion).  GUILE's API uses data that can be passed into C
> functions efficiently as parameters.  This means that the SCM type
> must be a machine word, so the genericity suggested by the GUILE docs
> are a joke.

Except that there are insiders and outsiders to the joke.  Do you really
consider yourself infallible so that you refuse to write code that would
be recognizably correct for someone doing a code review?

Code reviews are not fun.  And exactly because you want to buy yourself
time to work on more interesting and more valuable improvements, you
should strive to write code that can be reviewed by lesser people.
Those that actually read the APIs of Guile.  Or in this case, write code
that can be reviewed for correctness by the compiler.  The compiler is
not overly eager to write more interesting and more valuable
improvements, but it is a rather thorough and experienced reviewer.

> If you feel compelled to change large swaths of source code by
> substituting x == SCM_EOL with scm_is_eq(x, SCM_EOL), then I can't
> stop you, but to me it just looks like a waste of time.

That would be scm_is_null (x).  It is not exactly like the code gets
less readable by that substitution.

-- 
David Kastrup

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Likely a good frog project for someone with C knowledge

[Trevor Daniels]

Graham Percival wrote Wednesday, August 17, 2011 7:41 AM



On Tue, Aug 16, 2011 at 11:14:35PM -0300, Han-Wen Nienhuys wrote:
On Tue, Aug 16, 2011 at 7:17 PM, David Kastrup  
wrote:
> So what does relying on undefined behavior buy us apart from 
> the

> inability to debug type errors?

It buys us time to work on more interesting and more valuable 
improvements.


If she works on this,
asks questions about stuff she doesn't understand, and we add the
clarifications to the CG, I think this will be a great initial
Frog project.


+1

To me, the important first step is to enshrine the
correct constructs for common operations in the CG.
Looking through the source code to find examples
of coding Scheme/C++ interactions is confusing in
the extreme.  Even if this is unnecessary for correct
operation it is definitely worthwhile to establish
a LP convention.  We review contributions with
nitpicking comments on spelling and punctuation.  We
have policies on indentation.  Surely we should
establish a policy for coding these operations in
a standard (and correct) way.

Trevor




-
No virus found in this message.
Checked by AVG - www.avg.com
Version: 10.0.1392 / Virus Database: 1520/3838 - Release Date: 08/16/11


___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Likely a good frog project for someone with C knowledge

[Bertrand Bordage]

2011/8/17 David Kastrup 

> Bertrand Bordage  writes:
>
> > This would be great if Han-Wen decides to keep it like that.
> > Otherwise there is really a lot of work, with many shortcuts to
> > define.
> > to_boolean (scm_is_pair (x))
>
> That one would be wrong since scm_is_pair already returns a C boolean.
>

I was thinking about scm_pair_p ()...
This is typically the kind of mistakes that should be solved.
Shame on me, I won't do it again.

Where did you find SCM_CONSP ?
I don't see it in guile's reference. Maybe something to add to the CG ?

Bertrand
___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel