Replace C++ (in)equality checks with proper SCM syntax

Replace C++ (in)equality checks with proper SCM syntax

This commit replaces the most straightforward situations
where two SCM objects are compared.  Here are the basic
replacements I used:

x == SCM_BOOL_F            --->            scm_is_false (x)
x == SCM_BOOL_T            --->            to_boolean (x)
(I’d rather use something more straightforward such as
scm_is_true (x), but accordingly to the CG this is
The LilyPond Way®.)

x == (SCM y)               --->            scm_is_eq (x, y)
(often used with ly_symbol2scm)

I’m also adding some functions that Guile v1 does not provide:

x == SCM_EOL               --->            ly_is_eol (x)
x == SCM_UNDEFINED         --->            ly_is_undefined (x)
(scm_exact_p (x)) == SCM_BOOL_T   ---->    ly_is_exact (x)

Finally, I replaced (!cached) with (!SCM_UNPACK (cached))
in lily-guile-macros.hh, as has been suggested once by David.

Please note that this commit does not handle some situations
I’m less comfortable with (namely those involving scm_c_memq,
scm_assq or scm_hashq_get_handle), and some scm_*_p checks
for which there isn’t a ly_is_* replacement yet (to wit,
scm_hash_table_p, scm_promise_p, and scm_variable_bound_p).
Those should be dealt with later (preferably with help from
more experienced people than yours truly).

[Valentin Villenave, 2015-04-07 13:22:28]

Greetings everybody,
I realize that this is a rather large patch; at home I had divided it into
smaller, more manageable commits such as
0001-C-Scheme-consistency-use-ly_is_eol.patch
0002-C-Scheme-consistency-use-scm_is_false.patch
0003-C-Scheme-consistency-use-to_boolean.patch
0004-C-Scheme-consistency-use-SCM_UNPACK.patch
0005-C-Scheme-consistency-use-scm_is_eq.patch
0006-C-Scheme-consistency-use-ly_is_exact.patch
0007-C-Scheme-consistency-use-ly_is_undefined.patch

Not sure how to upload these to Rietveld without opening many different issues,
though.  Besides, even though the changes are numerous, they are tiny and pretty
straightforward for the most part.

Cheers,
Valentin.

[Dan Eble, 2015-04-08 01:38:21]

> x == SCM_EOL               --->            ly_is_eol (x)

But...

> Guile has scm_is_null which is a macro expanding to the direct
> comparison with SCM_EOL if the type debugging is switched off, and to
> something that works and compiles if it is on.
> [David K. in
http://lists.gnu.org/archive/html/lilypond-devel/2011-08/msg00655.html ]

[Valentin Villenave, 2015-04-08 06:58:48]

Syntax improvements

[Valentin Villenave, 2015-04-08 22:10:32]

Compile fix

[Valentin Villenave, 2015-04-08 22:46:01]

Additional typo cleanup

[Valentin Villenave, 2015-04-09 19:40:38]

Bugfix (but some regtests results require to be investigated further)

[Valentin Villenave, 2015-04-10 13:01:11]

One final (minor) syntax correction

[dak, 2015-04-10 13:16:16]

Man, this looks like some serious piece of work.  Got a headache merely
reviewing it.

https://codereview.appspot.com/226840043/diff/80001/lily/cue-clef-engraver.cc
File lily/cue-clef-engraver.cc (right):

https://codereview.appspot.com/226840043/diff/80001/lily/cue-clef-engraver.cc...
lily/cue-clef-engraver.cc:174: if (!scm_is_eq (glyph, prev_glyph_)
Have you overlooked the difference between `equal' and `eq' here?  This and the
following two need to use ly_is_equal instead.

https://codereview.appspot.com/226840043/diff/80001/lily/general-scheme.cc
File lily/general-scheme.cc (left):

https://codereview.appspot.com/226840043/diff/80001/lily/general-scheme.cc#ol...
lily/general-scheme.cc:228: Real r (scm_to_double (s));
This looks like an interesting conundrum.  scm_exact_p and ly_is_rational differ
in that ly_is_rational also admits infinite values (which LilyPond's Rational
type can represent), and the following if (isinf (r) ... relies on that
difference.  So it would appear that here indeed scm_is_false (scm_exact_p (...
would be called for rather than !ly_is_rational (...

https://codereview.appspot.com/226840043/diff/80001/lily/general-scheme.cc
File lily/general-scheme.cc (right):

https://codereview.appspot.com/226840043/diff/80001/lily/general-scheme.cc#ne...
lily/general-scheme.cc:474: if (scm_is_integer (arg) && ly_is_rational (arg))
In combination with scm_is_integer, again scm_is_true (scm_exact_p ... appears
to correspond better with the desired semantics.

https://codereview.appspot.com/226840043/diff/80001/lily/grob-property.cc
File lily/grob-property.cc (right):

https://codereview.appspot.com/226840043/diff/80001/lily/grob-property.cc#new...
lily/grob-property.cc:146: if (!ly_is_false (handle))
This is just if (scm_is_true (handle)) ...

scm_is_false and scm_is_true are complementary and work both by comparison with
SCM_BOOL_F.

https://codereview.appspot.com/226840043/diff/80001/lily/grob-property.cc#new...
lily/grob-property.cc:161: return ly_is_false (handle) ? SCM_EOL : scm_cdr
(handle);
Personally, I'd use return scm_is_pair (handle) ? scm_cdr (handle) : SCM_EOL
here since it more obviously does not call scm_cdr on a non-pair value.

However, given the semantics of scm_sloppy_assq both are perfectly equivalent,
and arguably such a replacement would be beyond the scope of this patch which
should preferably limit itself to _obvious_ equivalences rather than those
relying on semantic knowledge.

https://codereview.appspot.com/226840043/diff/80001/lily/include/lily-guile.hh
File lily/include/lily-guile.hh (right):

https://codereview.appspot.com/226840043/diff/80001/lily/include/lily-guile.h...
lily/include/lily-guile.hh:105: inline bool ly_is_false (SCM x) { return
scm_is_eq (x, SCM_BOOL_F); }
This one is perfectly equivalent to scm_is_false and consequently unnecessary. 
I have not bothered flagging every use of ly_is_false in this patch: the
compiler will do a better job of that once the definition is removed.

https://codereview.appspot.com/226840043/diff/80001/lily/lily-guile.cc
File lily/lily-guile.cc (right):

https://codereview.appspot.com/226840043/diff/80001/lily/lily-guile.cc#newcod...
lily/lily-guile.cc:567: if (scm_is_integer(k))
scm_is_integer(k) -> scm_is_integer (k)

https://codereview.appspot.com/226840043/diff/80001/lily/mark-engraver.cc
File lily/mark-engraver.cc (right):

https://codereview.appspot.com/226840043/diff/80001/lily/mark-engraver.cc#new...
lily/mark-engraver.cc:144: && ly_is_rational (m))
Again, scm_is_true (scm_exact_p seems like the better match.  I am not sure it
makes sense at all to check for exactness here, though.  But then it is probably
not the job of this patch to change that.

https://codereview.appspot.com/226840043/diff/80001/lily/multi-measure-rest-e...
File lily/multi-measure-rest-engraver.cc (right):

https://codereview.appspot.com/226840043/diff/80001/lily/multi-measure-rest-e...
lily/multi-measure-rest-engraver.cc:234: if (last && (scm_is_null
(last->get_property ("text"))))
Newly introduced parentheses seem less necessary after the change than before.

https://codereview.appspot.com/226840043/diff/80001/lily/pitched-trill-engrav...
File lily/pitched-trill-engraver.cc (right):

https://codereview.appspot.com/226840043/diff/80001/lily/pitched-trill-engrav...
lily/pitched-trill-engraver.cc:121: || scm_is_true (ev->get_property
("force-accidental"));
scm_is_true needs to be to_boolean here since the original compares with
SCM_BOOL_T rather than SCM_BOOL_F.

https://codereview.appspot.com/226840043/diff/80001/lily/tie-formatting-probl...
File lily/tie-formatting-problem.cc (right):

https://codereview.appspot.com/226840043/diff/80001/lily/tie-formatting-probl...
lily/tie-formatting-problem.cc:1203: spec.has_manual_delta_y_ = !ly_is_rational
(scm_car (entry));
Ugh.  I don't know what scm_inexact_p is supposed to do here: it would consider
1.0 and 1 to be different which seems weird.

Perhaps better translate literally to scm_is_true (scm_inexact_p (... and add a
/* TODO: check whether inexact? is an appropriate condition here */ comment.

https://codereview.appspot.com/226840043/diff/80001/lily/tie-specification.cc
File lily/tie-specification.cc (right):

https://codereview.appspot.com/226840043/diff/80001/lily/tie-specification.cc...
lily/tie-specification.cc:36: has_manual_delta_y_ = !ly_is_rational (pos_scm);
Same here.

https://codereview.appspot.com/226840043/diff/80001/lily/time-signature-engra...
File lily/time-signature-engraver.cc (right):

https://codereview.appspot.com/226840043/diff/80001/lily/time-signature-engra...
lily/time-signature-engraver.cc:107: if (time_signature_ && (!scm_is_null
(time_cause_)))
Unnecessary parens.  Were unnecessary before but become more so when using
function call instead of operator.

https://codereview.appspot.com/226840043/diff/100001/lily/volta-repeat-iterat...
File lily/volta-repeat-iterator.cc (right):

https://codereview.appspot.com/226840043/diff/100001/lily/volta-repeat-iterat...
lily/volta-repeat-iterator.cc:89: && (scm_is_null (current_reps) || scm_is_pair
(current_reps)))
This looks like (where && ly_cheap_is_list (current_reps))

[Valentin Villenave, 2015-04-10 14:06:11]

Use !scm_is_true instead of ly_is_false (and other suggestions from David)

[Valentin Villenave, 2015-04-10 14:23:39]

On 2015/04/10 13:16:16, dak wrote:
> Man, this looks like some serious piece of work.  Got a headache merely
> reviewing it.

Yeah, my head’s been hurting for a couple of days as well.

> Have you overlooked the difference between `equal' and `eq' here?

Totally. It’s fixed now (and everywhere else as well).

> This looks like an interesting conundrum.  scm_exact_p and ly_is_rational
differ
> in that ly_is_rational also admits infinite values (which LilyPond's Rational
> type can represent), and the following if (isinf (r) ... relies on that
> difference.  So it would appear that here indeed scm_is_false (scm_exact_p
(...
> would be called for rather than !ly_is_rational (...

... Or we could get a ly_is_exact function. (Same goes for scm_hash_table_p and
others.)
Notably, Guile v2 has all sort of scm_is_[stuff] tests built in (much more than
1.8).

> scm_is_false and scm_is_true are complementary and work both by comparison
with
> SCM_BOOL_F.

OK. So I’ll just use scm_is_true and !scm_is_true.

> However, given the semantics of scm_sloppy_assq both are perfectly equivalent,
> and arguably such a replacement would be beyond the scope of this patch which
> should preferably limit itself to _obvious_ equivalences rather than those
> relying on semantic knowledge.

Yep. This calls for an ulterior patch; I’m just aiming for the low-hanging
fruits here.

> lily/lily-guile.cc:567: if (scm_is_integer(k))
> scm_is_integer(k) -> scm_is_integer (k)

Done.

>
https://codereview.appspot.com/226840043/diff/80001/lily/mark-engraver.cc#new...
> lily/mark-engraver.cc:144: && ly_is_rational (m))
> Again, scm_is_true (scm_exact_p seems like the better match.  I am not sure it
> makes sense at all to check for exactness here, though.  But then it is
probably
> not the job of this patch to change that.

Indeed. That’s the sort of stuff that will get picked later anyway, like
Reinhold once suggested:
http://lists.gnu.org/archive/html/lilypond-devel/2011-08/msg00662.html

>
https://codereview.appspot.com/226840043/diff/80001/lily/multi-measure-rest-e...
> lily/multi-measure-rest-engraver.cc:234: if (last && (scm_is_null
> (last->get_property ("text"))))
> Newly introduced parentheses seem less necessary after the change than before.

Oh, yes. Done.

>
https://codereview.appspot.com/226840043/diff/80001/lily/pitched-trill-engrav...
> lily/pitched-trill-engraver.cc:121: || scm_is_true (ev->get_property
> ("force-accidental"));
> scm_is_true needs to be to_boolean here since the original compares with
> SCM_BOOL_T rather than SCM_BOOL_F.

Indeed. Plus, it broke a regtest as I found earlier.

> Perhaps better translate literally to scm_is_true (scm_inexact_p (... and add
a
> /* TODO: check whether inexact? is an appropriate condition here */ comment.

I’ve considered adding TODO and SIMPLIFYME comments, but anyway this will get
picked with a straightforward git grep.

>
https://codereview.appspot.com/226840043/diff/100001/lily/volta-repeat-iterat...
> lily/volta-repeat-iterator.cc:89: && (scm_is_null (current_reps) ||
scm_is_pair
> (current_reps)))
> This looks like (where && ly_cheap_is_list (current_reps))

Yes. I’m not familiar enough with ly_is_list (which is considered expensive) vs
ly_cheap_is_list (which basically checks for pairs IIUC).

I’ve added all of your suggestions; with a bit of luck the current patch can be
safely merged. Unlike earlier though, make check gives me six files changed,
which is two more than I’d be comfortable with:

input/regression/out-test/profile-property-access
input/regression/out-test/test-output-distance
input/regression/out-test/graphviz
input/regression/out-test/font-family-override
input/regression/out-test/tree
input/regression/midi/out-test/tree

Cheers,
Valentin.

[Valentin Villenave, 2015-04-10 19:51:46]

Use scm_is_true whenever possible

[Valentin Villenave, 2015-04-10 19:52:18]

Use scm_is_true whenever possible

[Valentin Villenave, 2015-04-11 07:14:53]

What a mess. I inadvertently uploaded my "round 2" proposal onto this issue
where it doesn’t belong.

Since Rietveld doesn’t seem to offer a way of undoing it, please disregard
"patch set #10" (#9 is the one actually being reviewed, with all of David’s
suggestions taken into account).

https://codereview.appspot.com/225380043 is the place where round 2 will be
discussed.

[dak, 2015-04-11 07:43:55]

v.villenave@gmail.com writes:

> What a mess. I inadvertently uploaded my "round 2" proposal onto this
> issue where it doesn’t belong.
>
> Since Rietveld doesn’t seem to offer a way of undoing it,

See the Button marked "Delete" in the following screen shot.

[Valentin Villenave, 2015-04-11 19:46:19]

On 2015/04/11 07:43:55, dak wrote:
> See the Button marked "Delete" in the following screen shot.

Thanks.

I’ve triple-checked this patch and it doesn’t appear to be breaking anything
(I’m running make check again just in case, but I’ll feel reassured once it’s
checked by other people as well); hopefully we can move forward and proceed to
the next one within a few days.

[pkx166h, 2015-04-21 12:07:25]

Patch counted down - please push