FWD » Core Development » Base Language

The solution would be to save the precision "by reference" (i.e. for each decimal array reference, save its precision). When ArrayAssigner.resize (and I think ArrayAssigner.copyOf) is called, do precision management by i.e. removing the precision for the old array and saving it for the new one.

The decimal precision is effectively lost when (1) unfixed decimal array is defined and later fixed with the EXTENT statement or when (2) a fixed array is reset to unfixed state again with the EXTENT statement or (3) during array assignment (whether simple assignment or passing array as function parameters).

The problem is that the precision information is not preserved for the type (array) definition, but only for the array components.

The solution, as already outlined above, is to store the precision information for each array instance and copy the precision information when one of the cases mentioned above occurs, i.e. the array instance is copied.

The affected methods are decimal.setPrecision(decimal[], int), ArrayAssigner.resize and ArrayAssigner.copyOf. These have to properly handle storage management of the precision information. ArrayAssigner already stores dynamic (indeterminate) arrays in a scopable work area as raw references. An implementation approach would be to extend the storage schema - instead of storing the raw reference, a wrapper instance would be stored containing the array reference and the precision information.

When the dynamic array is resized (EXTENT statement is encountered), old array reference is "deregistered" and new reference is "registered" from/to ArrayAssigner.WorkArea. The method ArrayAssigner.WorkArea.deregister contains a logic to deregister the reference from all scopes, but the register counterpart only registers to the scope on the top of the scope stack.

Can it ever happen to have the array reference registered to multiple scopes on the scope stack?

Can it ever happen to have the array reference registered to multiple scopes on the scope stack?

Yes. I believe this happens when it is passed as a parameter to a user-defined function, internal procedure or an external procedure.

Greg Shah wrote:

Can it ever happen to have the array reference registered to multiple scopes on the scope stack?

Yes. I believe this happens when it is passed as a parameter to a user-defined function, internal procedure or an external procedure.

I'll create a test case for this. Eventually the re-registration will have to be a bit smarter to register into all respective scopes.

Setting decimal precision in the DEFINE VARIABLE statement of an EXTENT with the missing number of array elements produces uncompilable Java code.

Example:

def var d1 as decimal extent decimals 1 init [1, 2, 3, 4, 5].
def var d2 as decimal extent 5 decimals 2 init [5,4,3,2,1].
def var d3 as decimal extent decimals 3.
def var d4 as decimal extent 1 decimals 4.
def var d5 like d1.
def var d6 like d2.

yields the following Java code (only relevant part shown):

         public void init()
         {
            ArrayAssigner.registerDynamicArray(d1);
            d1.setPrecision(1); // ERROR
            TransactionManager.register((Object) d1, (Object) d2, (Object) d3, (Object) d4, (Object) d5, (Object) d6);
            decimal.setPrecision(d2, 2);
            assignMulti(d3, new decimal(0));
            ArrayAssigner.registerDynamicArray(d3);
            d3.setPrecision(3); // ERROR
            assignMulti(d4, new decimal(0));
            decimal.setPrecision(d4, 4);
            ArrayAssigner.registerDynamicArray(d5);
            d5.setPrecision(1); // ERROR
            decimal.setPrecision(d6, 2);
         }

I am not sure how def var d3 as decimal extent 0 decimals 3. would behave, will test.

Hynek Cihlar wrote:

Setting decimal precision in the DEFINE VARIABLE statement of an EXTENT with the missing number of array elements produces uncompilable Java code.

When the number of array elements is missing, then we have a dynamic extent. And looks like there is a conversion problem here.

I am not sure how def var d3 as decimal extent 0 decimals 3. would behave, will test.

extent 0 marks a scalar variable.

Constantin Asofiei wrote:

When the number of array elements is missing, then we have a dynamic extent. And looks like there is a conversion problem here.

Created issue #2250.

extent 0 marks a scalar variable.

You are right, surprising behavior.

There's a bug in ArrayAssigner.assignMulti(BaseDataType[], BaseDataType[]). When assigning a determinate array to a non-determinate array, ArrayAssigner.resize is called allocating new array. But since the assignMulti method doesn't return the newly allocated instance, the assignment has in this case no effect.

Hynek Cihlar wrote:

There's a bug in ArrayAssigner.assignMulti(BaseDataType[], BaseDataType[]). When assigning a determinate array to a non-determinate array, ArrayAssigner.resize is called allocating new array. But since the assignMulti method doesn't return the newly allocated instance, the assignment has in this case no effect.

OK, good catch. Go ahead and fix it, you will need a conversion change too, so that it emits like this:

target = ArrayAssigner.assignMulti(target, source)

Hynek, about 0316a.zip. The implementation of isExpressionOfExtentType should check for extent annotations, not only for a KW_EXTENT child; as you already found, extent 0 defines a scalar var.

Open the generated .ast file and check the existing annotations for a variable definition, extent function, extent field, etc: there should be an extent annotation for all these cases. And I don't think you need to explicitly check for each case (var def, func, table field, etc): if the refid points to an AST with the extent annotation, then is enough to assume that this is an extent.

Constantin Asofiei wrote:

Hynek, about 0316a.zip. The implementation of isExpressionOfExtentType should check for extent annotations, not only for a KW_EXTENT child; as you already found, extent 0 defines a scalar var.

Open the generated .ast file and check the existing annotations for a variable definition, extent function, extent field, etc: there should be an extent annotation for all these cases. And I don't think you need to explicitly check for each case (var def, func, table field, etc): if the refid points to an AST with the extent annotation, then is enough to assume that this is an extent.

Yes I noted the extent annotation but wasn't sure I could rely on it in all the cases. I will change the implementation of isExpressionOfExtentType according to your suggestion. Unless you are against it, I will keep the isExpressionOfExtentType method even though the implementation simplified significantly.

Hynek Cihlar wrote:

Unless you are against it, I will keep the isExpressionOfExtentType method even though the implementation simplified significantly.

I don't think isExpressionOfExtentType is needed, just add the dereferencing code and the extent annotation check in the TRPL rules.

Hynek, the changes look good. How is the decimal precision working?

I have finished a release-candidate implementation for the decimal precision. I am testing it and also looking for edge-cases so it is in-line with Progress behavior.

The converted code doesn't give the correct error message when assigning a fixed array to an indeterminate and already fixed array. The correct message should be 'Indeterminate extent is already fixed to a dimension of x. (13738)' where x is the array length.

I fixed this in the ArrayAssigner as part of the decimal precision fix.

In Progress, function/procedure fixed-extent parameter's components are initialized to unknown values when an unfixed indeterminate extent is passed in. P2J on the other hand initializes the components to zero values.

The following sample prints "2 ? ?" in Progress but "2 0 0" in P2J.

function foo returns int (input-output j as int extent 2).
  message extent(j) j[1] j[2].
end.
def var i as int extent.
foo(input-output i).

When an extent is fixed using the extent statement, the components are initialized to zero values - both in Progress and P2J.

The following code sample causes the error message 'Calling procedure /home/hc/p34141_Untitled8.ped cannot input an indeterminate extent parameter if the sub-procedure foo /home/hc/p34141_Untitled8.ped parameter
is a fixed size extent. (11421)' in Progress but not in P2J. The execution is not interrupted.

function foo returns int (input p as int extent 2).
end.
def var k as int extent.
foo(input k).

Hynek Cihlar wrote:

The following code sample causes the error message 'Calling procedure /home/hc/p34141_Untitled8.ped cannot input an indeterminate extent parameter if the sub-procedure foo /home/hc/p34141_Untitled8.ped parameter
is a fixed size extent. (11421)' in Progress but not in P2J. The execution is not interrupted.

[...]

Add a pause at the end of the program to be sure the message is really not displayed.

Constantin Asofiei wrote:

Hynek Cihlar wrote:

The following code sample causes the error message 'Calling procedure /home/hc/p34141_Untitled8.ped cannot input an indeterminate extent parameter if the sub-procedure foo /home/hc/p34141_Untitled8.ped parameter
is a fixed size extent. (11421)' in Progress but not in P2J. The execution is not interrupted.

[...]

Add a pause at the end of the program to be sure the message is really not displayed.

The same with pause - the error message is not displayed in P2J.

Hynek Cihlar wrote:

Constantin Asofiei wrote:

Hynek Cihlar wrote:

The following code sample causes the error message 'Calling procedure /home/hc/p34141_Untitled8.ped cannot input an indeterminate extent parameter if the sub-procedure foo /home/hc/p34141_Untitled8.ped parameter
is a fixed size extent. (11421)' in Progress but not in P2J. The execution is not interrupted.

[...]

Add a pause at the end of the program to be sure the message is really not displayed.

The same with pause - the error message is not displayed in P2J.

Go ahead and fix this in ControlFlowOps$InternalEntryCaller.valid.

Hynek Cihlar wrote:

In Progress, function/procedure fixed-extent parameter's components are initialized to unknown values when an unfixed indeterminate extent is passed in. P2J on the other hand initializes the components to zero values.

This one I think requires a conversion change: integer[] j = extj.getVariable(2); needs to be converted to integer[] j = extj.getVariable(2, true); in case of input-output parameters, where the true flag means to set the extent array elements to unknown.

Constantin Asofiei wrote:

Go ahead and fix this in ControlFlowOps$InternalEntryCaller.valid.

For the function case, ControlFlowOps is not called. Perhaps it should be fixed somewhere in BlockManager.

Also I mentioned that the execution is not interrupted. To be more specific, the function body is not executed and the execution resumes just after the function call (!).

Hynek Cihlar wrote:

Constantin Asofiei wrote:
For the function case, ControlFlowOps is not called. Perhaps it should be fixed somewhere in BlockManager.

Also I mentioned that the execution is not interrupted. To be more specific, the function body is not executed and the execution resumes just after the function call (!).

Simple legacy function calls convert to java method calls, bypassing ControlFlowOps. But as the signature matches (in terms that both function call and definition pass an extent param), the java code will compile safely. I think the conversion rules should check the fact that either the func definition or the caller uses a dynamic extent argument and convert this into a ControlFlowOps.invokeFunction API call, instead of a java method call. BlockManager has no knowledge about the arguments passed by the caller, so it can' be used.

Hynek Cihlar wrote:

May I suggest the following solution? To revert the change in OutputExtentParameter and ArrayAssigner removing the responsibility of initializing the parameter from the getVariable method and to modify the conversion to emit the initialization code into the function block's init method. That is the init method would contain assignMulti(varName, decimal.instantiateUnknown()) for input-output params and assignMulti(varName, decimal.instantiateDefault()) for output params.

Go ahead, the approach looks OK.

Constantin Asofiei wrote:

Hynek Cihlar wrote:

Constantin Asofiei wrote:
For the function case, ControlFlowOps is not called. Perhaps it should be fixed somewhere in BlockManager.

Also I mentioned that the execution is not interrupted. To be more specific, the function body is not executed and the execution resumes just after the function call (!).

Simple legacy function calls convert to java method calls, bypassing ControlFlowOps. But as the signature matches (in terms that both function call and definition pass an extent param), the java code will compile safely. I think the conversion rules should check the fact that either the func definition or the caller uses a dynamic extent argument and convert this into a ControlFlowOps.invokeFunction API call, instead of a java method call. BlockManager has no knowledge about the arguments passed by the caller, so it can' be used.

Ok, I will modify the conversion rules to invoke any function with one or more input extent parameters through ControlFlowOps and also implement the "input parameter" validation in ControlFlowOps$InternalEntryCaller.valid.

Constantin Asofiei wrote:

Hynek Cihlar wrote:

May I suggest the following solution? To revert the change in OutputExtentParameter and ArrayAssigner removing the responsibility of initializing the parameter from the getVariable method and to modify the conversion to emit the initialization code into the function block's init method. That is the init method would contain assignMulti(varName, decimal.instantiateUnknown()) for input-output params and assignMulti(varName, decimal.instantiateDefault()) for output params.

Go ahead, the approach looks OK.

Sorry, I'm coming back on this one: when we can choose between emitting code and hiding it in the runtime, is best to hide it in the runtime. Thus, the assignMulti call which assigns either to default or unknown, should be hidden inside the OutputExtentParameter.getVariable. Before doing the changes, please post how the converted java code will look like, for INPUT, OUTPUT and INPUT-OUTPUT cases.

BTW, do your tests cover both function and procedure with extent parameters? We need to ensure the behaviour is the same in both cases, before doing the changes.

Constantin Asofiei wrote:

Sorry, I'm coming back on this one: when we can choose between emitting code and hiding it in the runtime, is best to hide it in the runtime. Thus, the assignMulti call which assigns either to default or unknown, should be hidden inside the OutputExtentParameter.getVariable. Before doing the changes, please post how the converted java code will look like, for INPUT, OUTPUT and INPUT-OUTPUT cases.

OK, no problem. I'll change the implementation back to the initialization being part of the array allocation. See the attached DynamicExtentParamInit.java showing how the converted code will look like for functions and procedures with input, input-output and output parameters. Note the call extj.setParameter(j, true) for the input-output case which initializes the array components to unknown values. The extj.getVariable(2) call for the output case initializes the array components to zero values. Also note the missing explicit array assignments in the init functions of the function and procedure blocks.

BTW, do your tests cover both function and procedure with extent parameters? We need to ensure the behaviour is the same in both cases, before doing the changes.

I include test cases with functions and internal procedures, no external procedures though.

Any update on the review request from the note #33?

Hynek Cihlar wrote:

Note the call extj.setParameter(j, true) for the input-output case which initializes the array components to unknown values.

Shouldn't the initialization be at the getVariable call, not the setParameter? Because getVariable is the API which creates it.

Any update on the review request from the note #33?

1. the javadoc for ArrayAssigner.resize initToUnknown is confusing/incomplete: Initialization flag, see above for more info., but there is nothing "above".
2. the compile error at ArrayAssigner.assignMulti:129
3. OutputExtentParameter is OK, as it adds the getVariable(int extent, boolean initToUnknown) API, and not the setParameter(T[] parameter, boolean initToUnknown) - so you can keep it.

Constantin Asofiei wrote:

Hynek Cihlar wrote:

Any update on the review request from the note #33?

I guess the update is still WIP, right? Because it doesn't compile and the disabled rule at convert/variable_definitions.rules:488 should be removed, if you no longer need it (now you have a false and condition, to disable it).

I think you reviewed the wrong file. The attached file in note 33 (hc_upd20140329a.zip) fixes the issue with extent to extent assignment for the cases when the passed in array gets resized.

The changes you reviewed was a request for review of the initial implementation of the extent array component initialization when passed in as a function/procedure parameter. Yes it doesn't compile and it lacks proper Javadocs as mentioned in the related note. I will continue on this initial implementation to provide the final solution. I think you can ignore it at this point.

Hynek Cihlar wrote:

I think you reviewed the wrong file.

Yes, it was the wrong file. What confused me was that I expected 0329b.zip to be an incremental update of 0329a.zip, that's why I looked at 0329b.zip.

• assignments.rules:222 to 236: these lines should be one indent to the right
• otherwise the changes look good. Have you already ran runtime testing with 0329a.zip update? If yes and it passed, go ahead and release it once you fix the formatting issue.

Hynek Cihlar wrote:

Yes, of course, sorry for that. The attached file is the correct example.

Constantin, is the latest DynamicExtentParamInit.java acceptable now?

Hynek Cihlar wrote:

Hynek Cihlar wrote:

Yes, of course, sorry for that. The attached file is the correct example.

Constantin, is the latest DynamicExtentParamInit.java acceptable now?

Yes, it looks OK.

Is there anything left to do on this task?

Yes, there is. I have an implementation in place, but because of the few secondary issues I found (which some of them are already solved) it is not yet stable.

The latest problem I found is that Progress is inconsistent in regard to honoring decimal precision in variable initializers and output parameters and I am working on a fix.

I created new issue for the error handling of input fixed extent parameters (as of the note 23), the issue number is #2292.

In Progress the variable initializer doesn't care much about the declared decimal precision, see below.

def var d1 as decimal extent decimals 1 init [1.55].
message "d1[1] = 1.55" d1[1] = 1.55.
d1[1] = 1.55.
message "d1[1] = 1.6" d1[1] = 1.6.

def var d2 as decimal decimals 1 init 1.55.
message "d2 = 1.55" d2 = 1.55.
d2 = 1.55.
message "d2 = 1.6" d2 = 1.6.

def var d3 as decimal decimals 1 init 1.55.
message "d3 = 1.55" d3 = 1.55.
d3 = d3.
message "d3 = 1.6" d3 = 1.6.

outputs:

d1[1] = 1.55 yes
d1[1] = 1.6 yes
d2 = 1.55 yes
d2 = 1.6 yes
d3 = 1.55 yes
d3 = 1.6 yes

In the converted code, first the initial value is set and then the variable is set with the declared precision. A possible solution could be to extend the logic of decimal to allow setting the precision for future value updates.

Hynek Cihlar wrote:

In Progress the variable initializer doesn't care much about the declared decimal precision, see below.

Is the variable initializer using some default decimal precision value, instead of the user-defined one?

I created a new issue #2294 for the initializer ignoring declared decimal precision - it is related not only to extents and it would otherwise block the fix for the subject of this issue.

• assignments.rules:
  - line 229 is too long
• decimal.java:
  - you have a tab on line 612
• ArrayAssigner:
  - WorkArea.dynamicArrayPrecisions - you can use the diamond to avoid re-using the generic definition, i.e. new HashMap<> (the line is too long)
  - WorkArea - for the new methods, re-check the javadoc formatting

Also, please commit your testcases to bzr (the testcases project).

The changes look good.

As a side note, when building testcases, instead of using this:

/* expect all 'yes' */
message "d1[1] = 1.1" d1[1] = 1.1.

if d1[1] <> 1.1 then message "test 'd1[1] = 1.1' is incorrect!".

This will avoid checking the screen/side-by-side comparison for non-UI features. And when you run the test, if a message is shown on screen/file, you know exactly where to look.

Constantin Asofiei wrote:

As a side note, when building testcases, instead of using this:
[...]
you might find it useful to write the test like this:
[...]

This will avoid checking the screen/side-by-side comparison for non-UI features. And when you run the test, if a message is shown on screen/file, you know exactly where to look.

Indeed this is more useful. Thanks for the tip!

0505a passed regression test and was committed to bzr revision 10524.