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
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. The
current V1.8 code uses the Guile API.
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.
4. We would be using this undocumented feature to carry forward for use
in Guile 2.0 in preference to a documented Guile API routine.
5. Overall result would obfuscated code, thanks for finding this David,
but I don't think it's a runner.
> It may cause a slight cell
> count hit in Guile 1.8,
Local testing showed the cell count hit is improved by Patch-set 2
compared with patch-set 1, or do you have different mileage on this one,
David?
> but we don't want Guile 1 to stick around until the
end
> of 2.17 anyway.
Yeah, but I would like to be able to get small bits of the Guile 2
conversion reviewed and implemented ahead of the a "big bang" cut-over
to using Guile 2 sometime during 2.17. Picking off small things like
this done cut down a potential massive workload for fellow developers
when the Guile-2 patch finally needs reviewing.
I notice David did something like this recently for a patch to avoid the
part-combiner code barfing in V2 because it Guile 2 now has a reserved
word "when" which was used as a variable name by the p/c scheme code.
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 ?
Ian
http://codereview.appspot.com/6458159/
_______________________________________________
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel
