Re: Replace C++ (in)equality checks with proper SCM syntax (issue 226840043 by address@hidden)

[dak]

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#newcode174
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#oldcode228
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#newcode474
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#newcode146
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#newcode161
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.hh#newcode105
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#newcode567
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#newcode144
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-engraver.cc
File lily/multi-measure-rest-engraver.cc (right):

https://codereview.appspot.com/226840043/diff/80001/lily/multi-measure-rest-engraver.cc#newcode234
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-engraver.cc
File lily/pitched-trill-engraver.cc (right):

https://codereview.appspot.com/226840043/diff/80001/lily/pitched-trill-engraver.cc#newcode121
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-problem.cc
File lily/tie-formatting-problem.cc (right):

https://codereview.appspot.com/226840043/diff/80001/lily/tie-formatting-problem.cc#newcode1203
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#newcode36
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-engraver.cc
File lily/time-signature-engraver.cc (right):

https://codereview.appspot.com/226840043/diff/80001/lily/time-signature-engraver.cc#newcode107
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-iterator.cc
File lily/volta-repeat-iterator.cc (right):

https://codereview.appspot.com/226840043/diff/100001/lily/volta-repeat-iterator.cc#newcode89
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))

https://codereview.appspot.com/226840043/