On 2012/09/06 17:20:59, Ian Hulin (gmail) wrote:
On 2012/09/03 07:25:20, Patrick McCarty wrote:
> On 2012/09/03 05:39:57, dak wrote:
> > On 2012/09/03 03:41:39, Patrick McCarty wrote:
> > > LGTM
> >
> > Let's not get this merged. ly_lily_module_constant
("module-variable") is
> > available in _both_ Guile 1.8 as well as Guile 2.0.
>
> Thanks for mentioning this; I didn't recall the Scheme procedure
being
available
> in 1.8, but I see now that it is.
>
1. (module-variable) is undocumented in Guile 1.8.7
Like 90% of the module system when viewed from Scheme. You are not
seriously proposing that this is less supported than internal (and
equally undocumented) API functions stringed together?
scm_sym2var is not documented. scm_module_lookup_closure is not
documented.
2. To use this we would need to re-write ly_module_lookup to do a
symbol lookup
of "module-variable" and then execute it with a scm_call2.
See my review and proposed patch on the same issue. Yes.
The current V1.8 code
uses the Guile API.
The part that is getting deprecated and was never documented as existing
in the first place. And has rather peculiar semantics.
3. My gut feeling is re-writing it this way would possibly involve a
performance
hit because of the extra call to scm_c_lookup within
ly_lily_module_constant
every time, probably as bad or worse than the increased cell counts
David noted
in patch-set 1.
ly_lily_module_constant memoizes. That's what "constant" in its name is
about. It gets looked up the first time through, and reused afterwards.
4. We would be using this undocumented feature
Reality check: the calls currently in use are both undocumented as well
as deprecated.
to carry forward for use in Guile
2.0 in preference to a documented Guile API routine.
module-variable _is_ documented.
5. Overall result would obfuscated code, thanks for finding this
David, but I
don't think it's a runner.
Reality check: ly_lily_module_constant is not obfuscated code. It is
used 64 times in LilyPond:
git grep -c ly_lily_module_constant|awk '/:/
{split($0,x,":");n+=x[2];};END{print n;}'
I'll add a TODO comment above the 1.8 shim scm_module_variable to flag
that it
can be removed once support to using LilyPond with Guile V1.8 is
dropped.
Are there any objections to this going to Patch_push ?
Yes. It is not like the use of ly_lily_module_constant would be arcane
or anything.
When we go Guilev2, we don't want to search for all the backward
compatibility. So I'd say you use something like
#if GUILEV1
for splicing in the compatibility stuff, and define it as either 0 or 1
in lily-guile.hh, depending on the conditional you use here.
Once we decide to go Guilev2 proper, we rip out the definition in
lily-guile.hh, and all uses of GUILEV1 will bomb out.
That way we at least don't overlook the compatibility crutches.
http://codereview.appspot.com/6458159/
_______________________________________________
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel
