Re: Issue 2758. ly_module_lookup caused deprecation warnings with Guile V2.06. (issue 6458159)

[ianhulin44]

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/