Implement passing values by reference to CLIPPER methods

[cpyrgas]

No description provided.

[RobertvanderHulst]

Comment from Matt Slay:

BTW - FoxPro also has "pass by reference".

From VFP Help:

By default, variables and arrays pass to user-defined functions by value. Therefore, changes made in the function to passed variables and arrays are not passed back to the calling program. However, you can pass variables and arrays by reference by prefacing variables and arrays with the at sign (@) as shown in the following line of code:

myFunction(@var1, @var2, ...)

[mattslay]

Robert created Issue # 67 on GitHub to handle track this matter...

Click here to see issues 67 on GitHib

[RobertvanderHulst]

Matt,
This has nothing to do with the '=' operator

[RobertvanderHulst]

Assigning the locals back to the param array has been implemented. Will now start work on handling the parameters by reference (assigning the values back to the original location) on the side of the calling code. The design has been made. Now we "only" have to implement is.

[RobertvanderHulst]

See https://github.com/X-Sharp/XSharpDev/blob/master/XSharp/src/Compiler/XSharpCodeAnalysis/LocalRewriter/LocalRewriter_ClipperCall.cs.
I am already detecting the calls. My proposal is to generate a new method in the class that does the methodcall that has byref parameters for all parameters that are passed by reference. Inside this new method we construct the parameter array, call the existing method and then after that method returns we assign the values from the parameter array back to the byref parameters.

[nvkokkalis]

The problem here is that the work cannot be done (entirely) at the lowering phase because at that point the REF or ADDROF have already been bound (and any related errors already generated). So, it is at least necessary to prevent resolving them at the expression binder when they are arguments to a CLIPPER method. However, they are normally bound before the binder determines that it will call a CLIPPER method (the resolved arguments are needed to determine the best candidate).

I need to think about this...

[RobertvanderHulst]

Do whatever it takes to make this work. We really need this, also for the FoxPro dialect.

[nvkokkalis]

I have a working implementation of this, but only with the REF keyword. The ADDROF ('@') does not work, and is very tricky (and nasty) to implement; it would require binding the arguments twice (which comes with an assorted speed penatly) before resolving the call, keeping any errors in temporaries and throwing the errors after the call is bound. Very error prone.

So, if we can live with just the REF implementation, I strongly suggest to leave @ alone...

[RobertvanderHulst]

Nikos,
Unfortunately this will not do. This is used in legacy code where the REF keyword did not exist. People were using the @ operator for this always.
Maybe at parse time we can automatically translate the @ to REF (except in the core dialect) and then in the binder do the opposite (translate REF back to @ when the receiving method wants an address or delegate )?

[nvkokkalis]

This won't help much because the compiler would still need to re-bind the arguments. I'll try to find another solution.

[RobertvanderHulst]

Nikos,
This construct (clipper calling convention with params passed by reference with the @ sign) will only be used in 'old' code. I think it would be OK to add a special compiler option that enables the automatic translation of the @ operator to REF in the parser. Would that help ?

[cpyrgas]

Robert, it still needs to be resolved in other cases to AddressOf though, since this also used a lot in old code...

[nvkokkalis]

OK, I now have a working implementation that takes care of ADDROF as well. It's rather ugly and hacky, but it works (I must do some improvements before pushing it).

It works by binding ADDROF normally and then un-binding it when the method is resolved. As you can see this is not very robust as it breaks the normal compilation flow, but it seems to be working. However, it will only work if the ADDROF can be correctly bound (i.e. generates no errors). Is this OK?

[RobertvanderHulst]

Ugly and Hacky is not really a problem for me. What could cause binding of ADDROF to fail ?
I expect that it should work with locals and fields.
Will it also work with memvars and database fields? I don't think so and that is fine.

Btw : I have changed the way in which memvars and aliased fields are processed in the last weeks. They are now transformed into identities with a special syntax and in BindXsIdentifier(where you added support for undeclared variables) I am now handling this special syntax and creating the XsVariableSymbol with a getter/setter for Memvars or Fields. The memvar identifier is now something like "Xs$MemVar->Name" and the Field identifiers are "Alias->FieldName" and "Xs$Field->FIeldName". When resolving identifiers I am checking for the alias operator inside the name . See
https://github.com/X-Sharp/XSharpDev/blob/master/XSharp/src/Compiler/XSharpCodeAnalysis/Binder/Binder_Expressions.cs#L666.

This change made the code in the transform phase much simpler because I no longer have to do special things for assignment operators and this also support the += , -= etc operators.

[nvkokkalis]

Sure, it's not a problem until the bugs it hides come out...
Anyway, it should work for locals and fields. Not for memvars and dbfields (the REF syntax should work, though).

I like your new approach for memvars/dbfields. It's better to keep the tree transformation as simple as possible and have all the logic in the binding phase.

[nvkokkalis]

In the end, memvars will not work even with the REF keyword because I changed the implementation due to problems with indexed items (e.g. array elements)...

[RobertvanderHulst]

Can you create a new branch and push this, so I can have a look ?

[nvkokkalis]

I want to debug the REF arguments to a CLIPPER constructor before pushing it. Currently the XNode is modified somewhere, so when it reaches the lowering phase it is not of the CtorCallContext type (!). This means that the REF behavior is not correctly detected...

[nvkokkalis]

No, it's not the constructor declaration, it's the CtorCallContext that gets changed into a PrimaryExpressionContext. When created at the tree transformation it's correct, but something happens later. I'm looking into it.

[nvkokkalis]

OK, found it. It is the xform of primaryExpression that forwards the CsNode, so this happens for all primary expressions (so, I changed the lowering code to handle it correctly).

I pushed to the main compiler branch. Please try it and let me know of any problems.

[RobertvanderHulst]

I am on my way to Phoenix, just met Fabrice at the airport, so it will take a while...

[RobertvanderHulst]

Nikos,
This seems to work, even for complex code like
if test(@A,b,@) .and. test2(a,@b,c)
The compiler generates several references to locals, and I have a problem understanding if this correct. But it seems to work, even when the locals are not USUALs but also when the locals are ints. Well done !

[nvkokkalis]

Yes, a REF local is generated for each REF argument; this is expected. The local is assigned a reference to the original argument, and is used in its place when constructing the params array. Then, it is assigned back the modified element when the call returns.

[RobertvanderHulst]

Nikos
I have added a “DO” statement last week. It should automatically pass values by reference ( that’s how it works...) . Would it be enough to “generate” the @ sign in the transformation phase ( for parameters that are simple identifiers) ?

[nvkokkalis]

Frankly, I'm not sure what the DO does. But if it ends up calling a CLIPPER function, then generating an @ or REF should be enough; with preference to the REF because its handling is slightly more elegant (compiler-wise).

[nvkokkalis]

Oh, and remember that the HasRefArguments of the xnode must be true!

[RobertvanderHulst]

I have added some code to also make this work for DO statements. Ticket closed !