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

[dak]

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/