[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: Fixes boolean/SCM confusions, part 1. (issue 4875054)
From: |
dak |
Subject: |
Re: Fixes boolean/SCM confusions, part 1. (issue 4875054) |
Date: |
Thu, 18 Aug 2011 15:46:43 +0000 |
http://codereview.appspot.com/4875054/diff/1/lily/ambitus-engraver.cc
File lily/ambitus-engraver.cc (right):
http://codereview.appspot.com/4875054/diff/1/lily/ambitus-engraver.cc#newcode177
lily/ambitus-engraver.cc:177: Rational sig_alter = !scm_is_false
(handle)
On 2011/08/18 14:18:45, Reinhold wrote:
On 2011/08/18 14:03:06, Cécile Hauchemaille wrote:
> So, might scm_is_pair also work ?
Yes, of course, that's the correct check...
It is a check that will work correctly. However, it is not idiomatic.
The description for assoc and its friends will state that assoc will
return #f when no association could be found. It does not state "will
return something that is not a pair", even though #f is the only
possible non-pair value.
So in order to match the check best to the description of the function,
I'd really check against false.
scm_is_pair will definitely work just as reliably for the computer, but
I think for humans, the check for false requires less thinking.
http://codereview.appspot.com/4875054/diff/8002/lily/bar-check-iterator.cc
File lily/bar-check-iterator.cc (right):
http://codereview.appspot.com/4875054/diff/8002/lily/bar-check-iterator.cc#newcode52
lily/bar-check-iterator.cc:52: if (scm_is_true (tr->get_property
("ignoreBarChecks")))
As I already explained: you can't replace to_boolean with scm_is_true,
since to_boolean( '() ) -> 0, whereas scm_is_true ( '() ) -> 1 (I
apologize for the mixture of X and Scheme syntax), and '() is
_definitely_ one value that needs to be interpreted as false in the
context of boolean properties.
http://codereview.appspot.com/4875054/diff/8002/lily/bar-check-iterator.cc#newcode64
lily/bar-check-iterator.cc:64: if (scm_is_true (tr->get_property
("barCheckSynchronize")))
Again, scm_is_true is wrong here.
http://codereview.appspot.com/4875054/
- Fixes boolean/SCM confusions, part 1. (issue 4875054), cecile . hauchemaille, 2011/08/18
- Re: Fixes boolean/SCM confusions, part 1. (issue 4875054), bordage . bertrand, 2011/08/18
- Re: Fixes boolean/SCM confusions, part 1. (issue 4875054), bordage . bertrand, 2011/08/18
- Re: Fixes boolean/SCM confusions, part 1. (issue 4875054), reinhold . kainhofer, 2011/08/18
- Re: Fixes boolean/SCM confusions, part 1. (issue 4875054), cecile . hauchemaille, 2011/08/18
- Re: Fixes boolean/SCM confusions, part 1. (issue 4875054), dak, 2011/08/18
- Re: Fixes boolean/SCM confusions, part 1. (issue 4875054), reinhold . kainhofer, 2011/08/18
- Re: Fixes boolean/SCM confusions, part 1. (issue 4875054), n . puttock, 2011/08/18
- Re: Fixes boolean/SCM confusions, part 1. (issue 4875054), dak, 2011/08/18
- Re: Fixes boolean/SCM confusions, part 1. (issue 4875054),
dak <=
- Re: Fixes boolean/SCM confusions, part 1. (issue 4875054), Carl . D . Sorensen, 2011/08/18
- Re: Fixes boolean/SCM confusions, part 1. (issue 4875054), cecile . hauchemaille, 2011/08/18
- Re: Fixes boolean/SCM confusions, part 1. (issue 4875054), dak, 2011/08/18
- Re: Fixes boolean/SCM confusions, part 1. (issue 4875054), Carl . D . Sorensen, 2011/08/18
- Re: Fixes boolean/SCM confusions, part 1. (issue 4875054), Graham Percival, 2011/08/18
- Re: Fixes boolean/SCM confusions, part 1. (issue 4875054), David Kastrup, 2011/08/19
- Re: Fixes boolean/SCM confusions, part 1. (issue 4875054), Bertrand Bordage, 2011/08/19
- Re: Fixes boolean/SCM confusions, part 1. (issue 4875054), David Kastrup, 2011/08/19
- Re: Fixes boolean/SCM confusions, part 1. (issue 4875054), Bertrand Bordage, 2011/08/19
- Re: Fixes boolean/SCM confusions, part 1. (issue 4875054), Han-Wen Nienhuys, 2011/08/19