lilypond-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: Replace C++ (in)equality checks with proper SCM syntax (issue 226840


From: v . villenave
Subject: Re: Replace C++ (in)equality checks with proper SCM syntax (issue 226840043 by address@hidden)
Date: Fri, 10 Apr 2015 14:23:39 +0000

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#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.

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-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.

Oh, yes. Done.


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.

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-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))

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.

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

reply via email to

[Prev in Thread] Current Thread [Next in Thread]