Issue 2758. ly_module_lookup caused deprecation warnings with Guile V2.06.

Issue 2758. ly_module_lookup caused deprecation warnings with Guile V2.06. The V2.17.0 definition of ly_module_lookup in module-scheme.cc uses two functions, scm_sym2var and scm_module_lookup_closure, which issue deprecation warnings in Guile V2.06.  A call to the new Guile API function, scm_module_variable, provides the functionality for both deprecated routines, but has not been back-ported to Guile V1.8.

This patch adds a conditionally-compiled shim function definition for
scm_module_variable when running with a Guile version < V2.0,
and changes ly_module_lookup to call scm_module_variable.

[Ian Hulin (gmail), 2012-08-19 22:31:46]

Cleaned up garbage in Subject and Description Fields.

[Graham Percival, 2012-08-21 11:51:53]

LGTM

[Patrick McCarty, 2012-09-03 03:41:39]

LGTM

[dak, 2012-09-03 05:39:57]

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.  It may cause a slight cell
count hit in Guile 1.8, but we don't want Guile 1 to stick around until the end
of 2.17 anyway.

[Patrick McCarty, 2012-09-03 07:25:20]

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.

> It may cause a slight cell
> count hit in Guile 1.8, but we don't want Guile 1 to stick around until the
end
> of 2.17 anyway.

Agreed.

Thanks,
Patrick

[Ian Hulin (gmail), 2012-09-06 17:20:59]

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

[dak, 2012-09-06 18:07:53]

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.

[Ian Hulin (gmail), 2012-09-06 23:24:04]

On 2012/09/06 18:07:53, dak wrote:
> 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?
> 

You need to modify your tone/attitude: you're being rude again.

> scm_sym2var is not documented.  scm_module_lookup_closure is not documented.
> 

Yes indeed, but unlike your proposal, I am not proposing to hide a documented
API function with an indirect call when running with Guile V2.

> > 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.
> 

Yes the title of the issue and this review did note that the current code caused
deprecation warnings for Guile V2.

> > 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.

Reality check: 
You want to hide a documented API call with an indirect call to a Guile REPL
level procedure *which* *is* *undocumented* *in* *Guile* *V1.8*. 
> 
> > to carry forward for use in Guile
> > 2.0 in preference to a documented Guile API routine.
> 
> module-variable _is_ documented.
> 

I checked in the Guile V1.8.7 documentation before posting my previous message. 
Please supply the reference as I don't appear to have looked carefully enough.

> > 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;}'
> 

Irrelevant.  The obfuscation I mean is using the indirect lookup needlessly in
Guile V2 when there is a perfectly adequate API routine to do the job directly.

> > 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.
> 

In this case it is.  (See above).

> 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.

Nice idea, but probably better to define the macro in guile-compatibility.hh,
which lily-guile.hh pulls in. I can then use as complementary GUILEV2 macro
which will be needed for the Guile V2-specific bits in main.cc.

[dak, 2012-09-08 09:13:17]

On 2012/09/06 23:24:04, Ian Hulin (gmail) wrote:
> On 2012/09/06 18:07:53, dak wrote:
> 
> > 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.
> 
> Nice idea, but probably better to define the macro in guile-compatibility.hh,
> which lily-guile.hh pulls in. I can then use as complementary GUILEV2 macro
> which will be needed for the Guile V2-specific bits in main.cc.

Sounds like a plan.  I guess neither of us are fond of the old use of
undocumented (and by now deprecated, as opposed to module-variable) internal
Guile functions.  We disagree about what to replace it with: I would use the
Scheme functions, saving the need for distinguishing Guilev1/Guilev2, you want
to use the C functions and keep different versions for Guilev1 and Guilev2.

If you implement the compatibility in the manner I propose (with your
modification), we are sure that the ugly code does not stick around after going
Guilev2-only.  So that's ok with me as well.  Having the GUILEv1 macro
temporarily defined will also help the transition elsewhere, and we should
probably also use a similar strategy at the Scheme level.

[Ian Hulin (gmail), 2012-09-08 17:03:34]

On 2012/09/08 09:13:17, dak wrote:
> On 2012/09/06 23:24:04, Ian Hulin (gmail) wrote:
> > On 2012/09/06 18:07:53, dak wrote:
> > 
> > > 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.
> > 
> > Nice idea, but probably better to define the macro in
guile-compatibility.hh,
> > which lily-guile.hh pulls in. I can then use as complementary GUILEV2 macro
> > which will be needed for the Guile V2-specific bits in main.cc.
> 
> Sounds like a plan.  I guess neither of us are fond of the old use of
> undocumented (and by now deprecated, as opposed to module-variable) internal
> Guile functions.  We disagree about what to replace it with: I would use the
> Scheme functions, saving the need for distinguishing Guilev1/Guilev2, you want
> to use the C functions and keep different versions for Guilev1 and Guilev2.
> 
> If you implement the compatibility in the manner I propose (with your
> modification), we are sure that the ugly code does not stick around after
going
> Guilev2-only.  So that's ok with me as well.  Having the GUILEv1 macro
> temporarily defined will also help the transition elsewhere, and we should
> probably also use a similar strategy at the Scheme level.

I'll upload another patch-set.  

Unfortunately things may slow down at my end again as I'm back teaching and
starting another round of mind-bending meds.

Cheers,
Ian

[Ian Hulin (gmail), 2012-09-25 11:23:30]

Closing Rietveld issue now that Tracker Issue 2758 has been verified.
Ian