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

[ianhulin44]

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.





http://codereview.appspot.com/6458159/