X-Sharp / XSharpPublic

Public repository for the source code for the XSharp Compiler, Runtime, Project System and Tools.
Apache License 2.0
113 stars 38 forks source link

Problems with using @ operator to pass vars by reference to CLIPPER methods #448

Closed cpyrgas closed 4 years ago

cpyrgas commented 4 years ago

Reported for consideration:

See the sample below:

-When trying to pass a var by reference late bound to a CLIPPER method (and with /vo7 enabled), then actually a pointer to that var is passed and the updated variable value assigned inside the method is not being passed back to the caller.

-When doing the same with early bound code, now the actual variable value (not pointer to the variable) is passed, but still the variable contents are not being updated in the caller

-When using "REF" instead of "@", then it works as expected (variable value passed and updated back to the caller)

Question of course is if using the "@" operator should indeed be used for passing vars by reference in this context. Pro argument is that this is what existing VO code does, con argument is that in this case it is not possible to pass a pointer to the var (although this scenario is probably far less common).

Maybe make it work so that when /vo7+ is enabled, @ always passes vars by reference, and when /vo7- is disabled, pass the pointer to the var? But in either case, the behavior needs to be consistent between early and late bound calls, which currently isn't.

// /vo7+
FUNCTION Start() AS VOID
    LOCAL o,u AS USUAL
    LOCAL t AS TestClass
    o := TestClass{}
    u := "OLD"

    ? "Late bound Clipper method with @:"
    ? "passed:", u
    o:ClipMeth(@u)
    ? "value after call:", u
    // OLD, <ptr value>, OLD

    ?
    t := TestClass{}
    u := "OLD"
    ? "Early bound Clipper method with @:"
    ? "passed:", u
    t:ClipMeth(@u)
    ? "value after call:", u
    // OLD, OLD, OLD

    ?
    ? "Early bound Clipper method with REF:"
    u := "OLD"
    ? "passed:", u
    o:ClipMeth(REF u)
    ? "value after call:", u
    // OLD, OLD, NEW
RETURN

CLASS TestClass
    METHOD ClipMeth(u)
        ? "received:", u
        u := "NEW"
    RETURN NIL
END CLASS
RobertvanderHulst commented 4 years ago

This was probably broken in a change that I did to support passing pointers inside usuals. However I have no idea how the compiler should "know" what the intention of the coder is for a late bound call that has a variable prefixed with @. But I agree that the early bound call with @ should work, and maybe the late bound call with REF as well, but that will be a pain to create I think. It is really very unfortunate that the VO devteam has chosen to overload the meaning of the @ operator.

By the way: since the FoxPro and XBase++ dialects to know know the "addressof" operator, then the early bound call with the @ sign should work without problems for these dialects.

cpyrgas commented 4 years ago

Robert, the REF version DOES work already for late bound calls! :) It is shown in the sample.

It's only the version with "@" that it doesn't. And I agree, there's no right and wrong way to do it, in both cases either passing the address, or passing by reference will not work correctly. But given that in 99% of the cases, in method calls the @ operator is used as a "by reference" operator, I guess it's best to make this work always this way, when /vo7 is enabled.

RobertvanderHulst commented 4 years ago

I checked and I cannot always change @ to REF when /vo7 is enabled. For example there are several locations in the SDK, like LogFile.prg line 272 where the function expects a pointer and in that case @ should pass the address and not REF.

This means that unfortunately I can't actually resolve this in the parsing stage but I have to wait until the function/method has been resolved by the Binder (RegCreateKeyEx in line 272) and then I can see if the function expects a pointer or a variable by reference.

The only other solution would be to change the code and store the address of the variable in another variable, like VAR hKeyAddress := @hKey and then pass that variable to the function without the @ prefix. But I am afraid that this will lead to a lot of "noise" by users that have similar code and (sometimes) don't understand the difference between using @ as an addressof operator and passing variables by reference using @.

cpyrgas commented 4 years ago

Robert, I expected this could be a thing. Not sure what we should do then. Probably just document it as an incompatibility with VO and noting that for CLIPPER method calls, the REF keyword must be used. After all, this is already a big improvement over vulcan, where passing vars by reference to CLIPPER methods was completely impossible...