FWD » Core Development » Base Language

Starting in Progress 10.x extent parameters are allowed in procedures and functions.

The solution is to use Java arrays for these kind of parameters; when using INPUT, OUTPUT and INPUT-OUTPUT mode, the same processing as for non-extent parameters can be used. In case the extent parameter is defined fixed-size, we can enforce this in Block.init (by emitting some assertion code which enforces the extent size to be the expected value). More, 4GL doesn't allow array elements to be passed to OUTPUT or INPUT-OUTPUT parameters (array elements are considered values).

Also, function return values can be defined as an array.

Do you think this is a good time to refactor BlockManager.*function APIs so that generics will be used ? Eventually if we will implement the function's "RETURN CLASS" clause, I think we will have no choice but to use generics. The only problem here is that we will need to return the array "by value" (i.e. a copy) and not the original array passed to the RETURN clause.

This means that extent variables (fields too?) can be passed to/returned from these blocks.

Yes, as long as the extent size is the same.

Another issue we will need to fix is how the GET-SIGNATURE reports such parameters - we will need to save in name_map.xml each parameter's extent size (if fixed). This will also be usefull when validating the parameter, as we will need to know the parameter's exent size (actually, we can emit the extent size directly in the generated code, as only fixed-size extent parameters will need special attention).

Do you think this is a good time to refactor BlockManager.*function APIs so that generics will be used ? Eventually if we will implement the function's "RETURN CLASS" clause, I think we will have no choice but to use generics.

It may be time for this. However, I worry about how much development time this will take, since we have a tight schedule and we are already late with milestone 4.

 The only problem here is that we will need to return the array "by value" (i.e. a copy) and not the original array passed to the RETURN clause.

We need to consider the implications. I wonder if the delegated assignment is needed here too (see #1621). Note that one problem for any modification of the original array (size and elements) is that if it was undoable, we may have some maintenance of the undo snapshots that would need to be done.

It may be time for this. However, I worry about how much development time this will take, since we have a tight schedule and we are already late with milestone 4.

Actually, this is quite easy: control_flow.rules needs a couple of changes to emit the BlockManager.function (or BlockManager.extentFunction in case of functions which return extent) API calls and to emit the return class (which is already computed). The other BlockManager changes are simple too. What is not that simple is to implement the function's RETURN statement to handle extent variables (but we would have to do this anyway).

• non-extent functions:
  

  <T extends BaseDataType> T function(Class<T>, Block)
  <T extends BaseDataType> T function(Class<T>, TransactionType, Block)
  

• fixed-size extent functions:
  

  <T extends BaseDataType> T[] extentFunction(Class<T>, int, Block)
  <T extends BaseDataType> T[] extentFunction(Class<T>, int, TransactionType, Block)
  

• dynamic-size extent functions:
  

  <T extends BaseDataType> T[] extentFunction(Class<T>, Block)
  <T extends BaseDataType> T[] extentFunction(Class<T>, TransactionType, Block)
  

• the new signature of functionBlock is:
  

   <T extends BaseDataType> T[] functionBlock(TransactionType level,
                                              Block           block,
                                              Class<T>        retType,
                                              int             extent)
  

  
  where extent will be -2 for non-extent functions, -1 for dynamic-extent functions, extent-size for other cases. In non-extent case, the returned value will be placed on the 0 element (which will be actually returned by the function APIs).

OK. Do it.

1. is cleaner to let -1 for non-extent, 0 for dynamic-extent and >0 for fixed-size extent (these constants will be used in other places, plus for array/parameters instantiation, i.e. will be possible to create an array of size zero - i.e. a non-initialized dynamic-extent variable)
2. A problem with extent as parameters is that we can no longer convert an 1-extent variable to a no-extent variable. This applies for fields too. This is because 1-extent vars/fields can be passed as parameters, plus they can be on the right side of an dynamic-extent-var = 1-extent-var statement.
3. All extent fields will need simple getter (which returns an array) because in 4GL it is valid:
   

   def temp-table tt1 field f1 as int extent 5.
   def var i as int extent.
   function f0 returns int (input j as int extent 5).
   end.
   
   f0(tt1.f1). /* to pass an extent field as parameter */
   i = tt1.f1. /* to be on the right-side of an assignment */
   

4. Please check if there are functions which return an extent, and the function is called using DYNAMIC-FUNCTION (and used in as a right-value in an assignment):
   

   function f0 returns int extent 2.
   end.
   
   def var i as int extent 2.
   i = dynamic-function("f0").
   

   
   If there are no such cases in current project, we can postpone this (else we will have to emit a ControlFlowOps.invokeDynamicExtentFunction in such cases and modify the ControlFlowOps runtime (which is not trivial).
5. must cast to Object any extent variable/field used in:
   - ControlFlowOps.invoke APIs - to not "explode" the elements of the array as individual elements of the varargs.
   - for TransactionManager.registerUndo/.register/.deregister, as these accept Undoables (and as I understand undo support for extent is done by saving each element from the array, not the array reference), we still need to change these APIs to work with varargs and handle arrays properly (as TM.register does)
6. any extent INPUT parameter will be initialized as in:
   

   integer[] extentParam = ArrayAssigner.copyOf(integer.class, _extentParam);
   

7. I think the right approach is to emit the extent parameter for any function/procedure as an OutputExtentParm (as you said at #1621).

1. properly convert 1-extent variables/fields
2. emit getter (in DMO iface and impl) for extent fields which returns an array, i.e. integer[] getF1()
3. fixed a problem in temp-table disambiguation, which didn't take into account the field's extent
4. emit dynamic-extent and initialize it as a zero-size array
5. refactored BlockManager.*Function APIs to use generics
6. added support for RETURN ext-var for functions returning extent
7. INPUT extent parameters use a copy determined using ArrayAssigner.copyOf
8. TM.deregister works with extent vars too

The good news is that there is no usage of user-defined functions returning an extent in any of the project code. The reason I know this is the bad news. When we updated the parser for 10.2b, we missed the addition of the EXTENT option to the function returns clause. So progress.g needs to be updated to support that syntax. If the project code (server or gui) had that construct, it wouldn't parse today. So we can defer that work, but when we do it we will need to update the func_return rule in progress.g.

In convert/variable_definitions.rules, on line 550, it states that all output parameters for functions will initialize to unknown. But, in OE 10.2B I think is no longer the case - maybe they changed something when they added extent param support ? If in 4GL 9.0 the real behavior is to initialize function's output parameter to unknown, then we need to add some config parameter to know the project's Progress version.

function func0 returns int (output i1 as int, output c1 as char).
  message "<" i1 "><" c1 ">". /* shows "<0> < >" */
end.

def var i as int.
def var c as char.
i = 10.
c  = "aa".

func0(output i,output c).

message "<" i "><" c ">". /* shows "<0> < >" */

We are already needing a similar feature for int64 support. v9 compiles some things to integer that v10 compiles to int64 (unless the -noint64 command line parameter is used).

I don't know if we can confirm that output parameter inits to unknown in v9. But let's operate on the assumption that it is correct and base our behavior on the proposed version flag. We may need some helpers that interpret the flag.

For example, we probably should put the specific progress version number in there (e.g. "9.1C" for Majic) but we will also want to be able to ask "is this v9?" instead of reading the string and parsing it in lots of different rule sets.

To be sure we don't brake anything in MAJIC, we will need to add this flag for this task (IMO, this is a conversion issue, and I will need to handle it now).

OK.

So progress.g needs to be updated to support that syntax. If the project code (server or gui) had that construct, it wouldn't parse today. So we can defer that work, but when we do it we will need to update the func_return rule in progress.g.

I've already done this yesterday, and it was easier to alter func_stmt rule so that it will save the extent at the KW_FUNCT node:

         fct:KW_FUNCT^ s:malformed_symbol!
         ftype = fr:func_return!
         (
            size = extent! 
            {
               if (size >= 0) ##.putAnnotation("extent",  new Long(size));
            }
         )?

OK. My only comment is that generally, I only drop nodes (using the !) that are "syntactic sugar". For example, DOT or END are things that are there to create the tree but are not needed to understand the syntax. I'm sure there are exceptions, but I prefer to leave the tree as much as possible as it was in the code.

There are 2 advantages to this:

1. It makes it easier to do reports when we leave behind the actual tree structure.

2. In the future, we will probably write an anti-parser for Progress. This would take a Progress AST and emit 4GL source code. This would be much easier if the original code was there. The point of doing this would be to use TRPL to do refactoring for 4GL projects.

I was dropping it because I think it was emitting the extent literal (if present) in the generated code. I will modify to hide this node in post-parse-fixups phase or annotations phase, before it gets a chance to be emitted.

 I have an parser warning for the getSourceCodeVersion function in common-progress.rules

Please provide details of the error.

Note that when compiling progress.g you will have warnings as now the extent rule returns a value.

Please provide details of the warnings. We will want to eliminate them so that real problems can be easily see in the build.

our validation code will not compile. More, in P2J we have no validator set per the extent field, the validation expression is still set for the element on index 1. 

The individual element references inside a validation expression is a known problem. From our conversion reference:

The result works the same way as all other validation expression processing. The reason is simple: the validateExpression() method always receives a proposed value that is a scalar instance (the proposed value is not a Java array), so this syntax works naturally in the converted code.

If the 4GL were to contain an array subscript, it would not work properly. See the Limitations section for details.

I will do a code review later today.

And this:

A special case occurs with variable or field array references (extents). In Progress, the specific extent element can be referenced without subscript when it is in a validation expression. This works in the converted code (in fact it is the only form that can work right now). If the code has an explicit subscript, then it will convert to code that will access the original instance of the variable/field instead of accessing the passedin scalar instance to test against. That will mean that such validation expressions would never work properly.

If you can fix this with small effort, go ahead.

The validation registration processing problem was not known, but I guess I am not surprised. Again, if you can fix it quickly, then go ahead.

Please provide details of the error.

The warning is in red and it says:

------------------------------------------------------------------------------
Core Code Conversion
------------------------------------------------------------------------------

Optional rule set [customer_specific_conversion] not found.
line 1:1: unexpected token: 2347

Please provide details of the warnings. We will want to eliminate them so that real problems can be easily see in the build.

    [antlr] .../progress.g:16836:12: warning:Rule 'extent' returns a value
    [antlr] .../progress.g:16904:12: warning:Rule 'extent' returns a value
    [antlr] .../progress.g:11584:12: warning:Rule 'extent' returns a value
    [antlr] .../progress.g:20124:12: warning:Rule 'extent' returns a value
    [antlr] .../progress.g:21891:34: warning:Rule 'extent' returns a value

I will look at the validation issues tomorrow and check how easy is to fix.

Note that I have some MAJIC-related files which I will need to change, as BlockManager.*function APIs have changed.

I have reviewed the proposed ruleset changes. They look good. I'll look at the runtime changes tomorrow.

Did you resolve the getSourceCodeVersion TRPL error? From the text, the problem is not obvious. I will have to consider it some more.

Did you resolve the getSourceCodeVersion TRPL error?

No, because the next step would have been to start checking the TRPL expression parser, and I didn't want to spend time on that now.

1. add a validation expression to each array element, in case of update i validate(...) where def var i as int extent 5. This is what I found during testing:
   

   def var i as int extent 3.
   update i validate(i[1] = 2, "err") with frame f1.
   

   
   When update is called, enter 2 for element on index 1 and another value for element on index 2. As no input clause was used in the validation expression, it checks the i[1] from the variable and not from the frame buffer. As i[1] is zero, validation will fail for element on index 2. Thus, I think 4GL adds the validation expression to each array element.
2. the interesting part is when variables are used as subscript in the validation expression, as in:
   

   def var i as int extent 3.
   def var j as int init 1.
   update i validate(i[j] = 2, "err") with frame f1.
   

   
   In this case, 4GL can't map i[j] to a specific widget, thus it uses the extent variable. In this example, no matter what value you set to element on index 1, it will not validate, as the pre-update i[j] is 0. If the validation expression is changed to input i[j] = 2, then widget value is forced and element on index 1 will pass validation when is set to 2.
3. the validation expression for each array element must be distinct, as it needs to disambiguate between i[<idx>] when <idx> is for the current element and i[<idx>] when <idx> is for another element (only when <idx> is a num literal). Thus, the following code:
   

   def var i as int extent 3.
   update i validate(i[1] = i[2], "err") with frame f1.
   

   
   needs to be converted as:
   

   ...
   f1Frame.widgetiArray0().setValidatable(new Validation1());
   f1Frame.widgetiArray1().setValidatable(new Validation2());
   f1Frame.widgetiArray2().setValidatable(new Validation3());
   ...
      private class Validation1
      extends Validator
      {
         public logical validateExpression(BaseDataType _newValue_)
         {
            final integer _i = (integer) _newValue_;
            /* as this validation expression is for element 1, all i[1] need to be substituted with the _newValue_ */
            return isEqual(_i, subscript(i, 2)); 
         }
         ...
      }
      private class Validation2
      extends Validator
      {
         public logical validateExpression(BaseDataType _newValue_)
         {
            /* as this validation expression is for element 2, all i[2] need to be substituted with the _newValue_ */
            final integer _i = (integer) _newValue_;
            return isEqual(subscript(i, 1), _i);
         }
         ...
      }
      private class Validation3
      extends Validator
      {
         public logical validateExpression(BaseDataType _newValue_)
         {
            /* as this validation expression is for element 3 and that is not used in the validation expression, then elements 1 and 2 are refered directly */
            return isEqual(subscript(i, 1), subscript(i, 2));
         }
         ...
      }
   

4. when using INPUT clause in the validation expression, it will convert to frame widget getters, which work OK even with extent fields.

Code feedback:

1. The OutputExtentParameter getVariable(int) method should be located above the private methods in the file, instead of leaving it at the end, below the inner classes.

2. The ArrayAssigner.lengthOf() passes a null or an int to the new integer() constructor. I presume that javac will determine that this should use the integer(Number) constructor that will properly deal with null as unknown. BUT, this seems like too much hidden dependencies. Please make this code less dependent by explicitly returning new integer() or new integer(int) depending on the situation.

3. In FieldReference, I think the getExtent() method name is misleading. It suggests that it will report the "extent" (size) of the field, but really it is just a artificial construct needed because Java cannot have polymorphic methods based only on return values. Perhaps Eric has an idea of a name he would prefer.

Otherwise, it all looks really good.

List of rules describing behavior of mixed extent and dynamic-extent in parameters and assignments:

1. output mode:
A. parameter is dynamic-extent
invoked simple:
- passed variable is dynamic-extent, not initialized: OK
- passed variable is dynamic-extent, initialized: Err#1
- passed variable is fixed-extent: Err#1
invoked dynamically: the same
B. parameter is fixed-size
invoked simple:
- passed variable is dynamic-extent, not-initialized: OK
- passed variable is dynamic-extent, initialized to same extent: OK
- passed variable is dynamic-extent, initialized to different extent: Err#1
- passed variable is fixed-extent (same size): OK
- passed variable is fixed-extent (different size): Compiler error
invoked dynamically (differences):
- passed variable is fixed-extent (different size): Err#1

2. input mode:
A. parameter is dynamic-extent - everything works
B. parameter is fixed-size
invoked simple:
- passed variable is dynamic-extent, not-initialized: Err#2
- passed variable is dynamic-extent, initialized to same extent: Ok
- passed variable is dynamic-extent, initialized to different extent: Err#1
- passed variable is fixed-extent (same size): OK
- passed variable is fixed-extent (different size): Compiler ERror
invoked dynamically (differences):
- passed variable is fixed-extent (different size): Err#1

3. input-output mode:
A. parameter is dynamic-extent - everything works
B. parameter is fixed-size
invoked simple:
- passed variable is dynamic-extent, not-initialized: OK
- passed variable is dynamic-extent, initialized to same extent: Ok
- passed variable is dynamic-extent, initialized to different extent: Err#1
- passed variable is fixed-extent (same size): OK
- passed variable is fixed-extent (different size): Compiler ERror
invoked dynamically (differences):
- passed variable is fixed-extent (different size): Err#1

4. function return definition is dynamic-extent:
A. returns dynamic-extent var, not initialized:
- assigned to dynamic-extent var, not initialized: OK
- assigned to a dynamic-extent var, initialized: Err#3
- assigned to a fixed-extent var: Err#4
B. returns dynamic-extent var, initialized:
- assigned to dynamic-extent var, not initialized: OK
- assigned to a dynamic-extent var, initialized same size: OK
- assigned to a dynamic-extent var, initialized different size: Err#3
- assigned to a fixed-extent var, same size: OK
- assigned to a fixed-extent var, different size: Err#5
C. returns fixed-extent var:
- assigned to dynamic-extent var, not initialized: OK
- assigned to a dynamic-extent var, initialized same size: OK
- assigned to a dynamic-extent var, initialized different size: Err#3
- assigned to a fixed-extent var, same size: OK
- assigned to a fixed-extent var, different size: Err#5

5. function return definition is fixed-extent:
A. returns dynamic-extent var, not initialized:
- assigned to dynamic-extent var, not initialized: OK
- assigned to a dynamic-extent var, initialized: Err#3
- assigned to a fixed-extent var, same size as func def return: Err#4
- assigned to a fixed-extent var, different size than func def return: Compiler Err#5
dynamic-function case:
- assigned to a fixed-extent var, different size than func def return: Err#4
B. returns dynamic-extent var, initialized:
- assigned to dynamic-extent var, not initialized: OK
- assigned to a dynamic-extent var, initialized same size as returned var: OK
- assigned to a dynamic-extent var, initialized different size than returned var: Err#3
- assigned to a fixed-extent var, same size as func def return, but different than the returned var extent: Err#5
- assigned to a fixed-extent var, same size as func def return, same as the returned var extent:OK
- assigned to a fixed-extent var, different size than func def return, same as the returned var extent:Compiler Error #5
- assigned to a fixed-extent var, different size than func def return, different than the returned var extent:Compiler Error #5
dynamic-function case:
- assigned to a fixed-extent var, different size than func def return, same as the returned var extent:OK
- assigned to a fixed-extent var, different size than func def return, different than the returned var extent:Error #5
C. returns fixed-extent var:
- assigned to dynamic-extent var, not initialized: OK
- assigned to a dynamic-extent var, initialized same size: OK
- assigned to a dynamic-extent var, initialized different size: Err#3
- assigned to a fixed-extent var, same size: OK
- assigned to a fixed-extent var, different size: Compiler Error Err#5
dynamic-function case:
- assigned to a fixed-extent var, different size: Err#5

6. extent-to-extent var assignment
A. left-side is dynamic-extent, not initialized
- right-side is dynamic-extent, not-initialized: OK
- right-side is dynamic-extent, initialized: OK
- right-side is fixed-extent: OK
B. left-side is dynamic-extent, initialized
- right-side is dynamic-extent, not-initialized: Err#3
- right-side is dynamic-extent, initialized, same size: OK
- right-side is dynamic-extent, initialized, different size: Err#3
- right-side is fixed-extent, same size: OK
- right-side is fixed-extent, different size: Err#3
C. left-side is fixed-extent
- right-side is dynamic-extent, not-initialized: Err#4
- right-side is dynamic-extent, initialized, same size: OK
- right-side is dynamic-extent, initialized, different size: Err#5
- right-side is fixed-extent, same size: OK
- right-side is fixed-extent, different size: Compiler Error Err#5

7. subscript usage
A. subscript is constant, in bounds.
- var is dynamic-extent, not initialized: Err#6
- var is dynamic-extent, initialized: OK
- var is fixed-extent: OK
B. subscript is constant, outside bounds.
- var is dynamic-extent, not initialized: Err#6
- var is dynamic-extent, initialized: Err#7
- var is fixed-extent: Compiler error
C. subscript is var, in bounds
- var is dynamic-extent, not initialized: Err#6
- var is dynamic-extent, initialized: OK
- var is fixed-extent: OK
D. subscript is var, outside bounds
- var is dynamic-extent, not initialized: Err#6
- var is dynamic-extent, initialized: Err#7
- var is fixed-extent: Err#8
E. subscript is unknown var
- var is dynamic-extent, not initialized: OK
- var is dynamic-extent, initialized: OK
- var is fixed-extent: OK
F. subscript is var, set to 0
- var is dynamic-extent, not initialized: Err#8
- var is dynamic-extent, initialized: Err#8
- var is fixed-extent: Err#9

Err#1 (no error raised, just message unless NO-ERROR clause is present):

Extent parameter dimension of <var-extent> from procedure <source-proc:name> is mismatched with sub-procedure <fname> <this-proc:name> parameter dimension of <par-extent> ...(328) (11428)

Err#2 (no error raised, just message unless NO-ERROR clause is present):

Calling procedure <source-proc:name> cannot input an indeterminate extent parameter if the sub-procedure <fname>  <this-proc:name> parameter is a fixed size extent. (11421)

Err#3 (error is raised)

Indeterminate extent is already fixed to a dimension of <var-extent>. (13738)

Err#4 (error is raised)

Uninitialized array used as source of assignment. (14906)

Err#5 (error is raised)

Whole-array assignment target and source must have the same extent unless the target is indeterminate. (14905)

Err#6 (error is raised)

Invalid runtime use of indeterminate extent.  It must be set to a fixed dimension. (11387)

Err#7 (error is raised)

Array subscript <idx> is out of range.  The indeterminate extent is fixed to a dimension of <var-extent>. (11388)

Err#8 (error is raised)

** Array subscript <idx> is out of range. (26)

Err#9 (error is raised)

** Array subscript 0 is out of range. (26)
** Unable to update  Field. (142)

Wow. Great work!

If we are far off from this implementation, then we will need to defer the runtime work until later. If close, then we can fix the runtime parts. I consider "close" as effort of 1 day or less.

In regard to the extent rule usage in progress.g, it seems that the following places can match the extent usage:

1. def_var_stmt
2. def_parm_stmt which calls regular_parm (where extent is used)
3. def_prop_stmt
4. constructor_stmt which calls run_stmt_parms with defineVariable true which calls parameter with defineVariable true where extent can used
5. method_stmt (same call tree as constructor stmt)
6. func_stmt (same call tree as constructor stmt)
7. getter_setter (same call tree as constructor stmt)

All of these places can define an extent var/parm and they all need to get an "extent" annotation if there is size >= 0. I assume that you are already doing this, but I just want to confirm it. The annotation needs to be created at the node that would be matched in the "var_def" function in common-progress.rules. I am putting together better reports for the various extent cases and I want to be able to rely upon this annotation.

I'm implementing these rules from last to first, and not obvious cases I'll leave last. About the validation problems: once I found the correct place to fix this (in annotations/validation_post.rules), the fix was straightforward.

About progress.g's extent rule - I am on the way of moving all code from downPath("KW_EXTENT")-like usage to "extent" annotation.

1. def_var_stmt

No special work needed, define_stmt calls sym.annotateVariableOptions which sets the extent annotation

2. def_parm_stmt which calls regular_parm (where extent is used)

I explicitly added the extent annotation.

3. def_prop_stmt

Same as def_var_stmt

4. constructor_stmt which calls run_stmt_parms with defineVariable true which calls parameter with defineVariable true where extent can used
5. method_stmt (same call tree as constructor stmt)
6. func_stmt (same call tree as constructor stmt)
7. getter_setter (same call tree as constructor stmt)

All these cases are handled by parameter rule, which calls sym.annotateVariableOptions, which sets the extent annotation.

1. subscript usage
2. extent-to-extent assignment
3. extent-to-var assignment
4. I've added rules to register each dynamic-extent array with ArrayAssigner, so that we can know which array is dynamic-extent and which one is not (even after the dynamic-extent has set its size).

I'll see what else I can finish in the next couple of hours and I'll place a final update today.

Feedback:

1. The int members of enum ArgValidationErrors in ControlFlowOps need javadoc.

2. The progress.g needs comments about how the upstream code will call sym.annotateVariableOptions() and that code will see the EXTENT node and will properly honor setting the extent annotation for def_var_stmt, def_prop_stmt... otherwise someone reading the code might think that the discarded size var means that the annotation will not be made.

3. I think there is an essential problem with the progress.g changes. It is valid in the 4GL to have EXTENT 0. This doesn't mean that the var is an indeterminate length array. This means that the var is scalar and not an array. But for us, the 0 means that it is an indeterminate length array. Please see the 4GL docs for define var where it describes this. Note that the current project DOES have several cases of EXTENT 0. This is important because the use of AS of LIKE can require the extent to be "removed" by explicitly using EXTENT 0.

About issue 3 - I chose 0 to represent dynamic extent because this will produce a valid reference in the converted code, i.e. a 0-length array; the alternative would have been to initialize the array variable with null, and I didn't like that. I think for these 0-extent cases there is no way but to drop (or hide, if it works) the KW_EXTENT node, and leave an annotation behind that the KW_EXTENT node was dropped/hidden.

I think for these 0-extent cases there is no way but to drop (or hide, if it works) the KW_EXTENT node, and leave an annotation behind that the KW_EXTENT node was dropped/hidden.

Yes, I agree it must be dropped and the resulting var should be treated as scalar.

Code Review Feedback:

This version looks good. I am fine with this, however I have questions:

1. I assume you have resolved the TRPL expression parsing error? If not, then we must solve this before going into test.
2. Are any of the listed problems above going to cause regressions in Majic?

1. I assume you have resolved the TRPL expression parsing error? If not, then we must solve this before going into test.

The problem is gone in latest version, and after a quick check, looks like the root cause was with a return variable named "digit". If I changed this code:

      <function name="is_digit">
         <parameter name="str"   type="java.lang.String" />
         <return    name="digit" type="java.lang.Boolean" />

         <rule>
            digit = str != null and 
                    (str == "0" or str == "1" or str == "2" or str == "3" or str == "4" or 
                     str == "5" or str == "6" or str == "7" or str == "8" or str == "9")
         </rule>
      </function>

      <function name="is_digit">
         <parameter name="str"   type="java.lang.String" />
         <return    name="digit1" type="java.lang.Boolean" />

         <rule>
            digit1 = str != null and 
                    (str == "0" or str == "1" or str == "2" or str == "3" or str == "4" or 
                     str == "5" or str == "6" or str == "7" or str == "8" or str == "9")
         </rule>
      </function>

2. Are any of the listed problems above going to cause regressions in Majic?

I don't expected to, but I will have a good answer (conversion-wise) tomorrow morning. And the schema validation problem with extent fields I don't think it is used in MAJIC.

The code looks good. Assuming your tests show that it needs no further changes, the plan is as follows:

1. As soon as the current changes (being tested in staging) are checked into bzr, merge up to that level.
2. Apply it to staging, reconvert and run the harness.

What I'm currently reconverting doesn't fix the dead-vars emitted in validateExpression and validateMessage methods. Let me know if I should fix this before regression testing in staging (I've tried to duplicate it using a standalone testcase, but is not that obvious and I wasn't able to duplicate it).

Do these problems already exist in Majic today?

OK, I've checked again and looks like I've been miss-interpreting the diff result, this update actually solves the issue of these dead-vars (which are in MAJIC today).

Unfortunately the conversion on the linux box crashed when it was almost at the end, so I will not have a conclusion about conversion regression testing until tomorrow morning.

FYI, there are some javadoc errors. You can fix them later (in a different check in).

 [javadoc] /mnt/san/sata/gc/20121029/p2j/src/com/goldencode/p2j/util/OutputExtentParameter.java:43: warning - Tag @link: can't find get(int) in com.goldencode.p2j.util.OutputExtentParameter
  [javadoc] /mnt/san/sata/gc/20121029/p2j/src/com/goldencode/p2j/util/OutputExtentParameter.java:40: warning - Tag @link: can't find get(int) in com.goldencode.p2j.util.OutputExtentParameter

ca_upd20130218a.zip has been committed as 10175 (has passed regression testing).

1. for extent fields, all elements get their distinct validation inner class
2. previously not-emitted validation inner classes are now emitted (this should clean with Ovidiu's related work, i.e. don't emit validation inner classes if they are not needed)
3. previously emitted validation code is now dropped
   and more

I'll review each one and see why this is happening, and I'll update here.

I'm OK with the changes. If all of the Majic modifications appear to be correct, we will go ahead with runtime testing of this update by itself.

Code Review:

My only question is this: the schema_validation.rules includes support for the INSERT statement, but then the FORMAT_PHRASE node is removed (which would presumably contain the validation expression) from insert_statement_expansion.rules.

Otherwise, I don't have any problems with the code changes.

My only question is this: the schema_validation.rules includes support for the INSERT statement, but then the FORMAT_PHRASE node is removed (which would presumably contain the validation expression) from insert_statement_expansion.rules.

The insert_statement_expansion.rules duplicates the entire KW_INSERT node, to create the new KW_CREATE node (this includes the FORMAT_PHRASE). If not removed from the KW_CREATE, it would end up with a FORMAT_PHRASE, which is not correct. For the KW_UPDATE, again the entire KW_INSERT is duplicated, which includes the FORMAT_PHRASE, and will remain attached to this node.

This makes sense. I should have read the surrounding code more carefully and I would have seen this.

If it passes conversion testing AND if you deem the Majic changes to be correct, then go ahead and check in/distribute.

If it passes conversion testing AND if you deem the Majic changes to be correct, then go ahead and check in/distribute.

All changes are caused by the fact that the FORM doesn't trigger schema validation expression registration anymore. Separate runtime regression testing is still recommended. Commmitted to bzr revision 10298.

What is to do on this task, if anything?

The only TODO is this:

- adds a TODO in decimal.setPrecision, for dynamic-extent vars - for these cases, the precision needs to be saved "by reference" and each time the dynamic-array's reference changes, the precision needs to be set again.

Please create a separate task for this remaining TODO and we will close this. Make sure to put a time estimate in there and add me as a watcher. Set the target version for milestone 11.