View Issue Details

IDProjectCategoryView StatusLast Update
0036909FPCCompilerpublic2020-09-19 16:19
ReporterRyan Joseph Assigned ToSven Barth  
PrioritynormalSeverityminorReproducibilityN/A
Status closedResolutionfixed 
Product Version3.3.1 
Fixed in Version3.3.1 
Summary0036909: [PATCH] Static array initialization from array constructor
DescriptionIt appears to be a very trivial task to get the compiler to accept array constructors to initialize static arrays (a long wished for feature for me) so I made a first draft attempt at implementing this, although I'm not sure how to handle managed types.

Can anyone explain what needs to happen conceptually when an array constructor with managed types is assigned to a static array? For static to static array assignments I think only fpc_copy_proc is called and that is sufficient to handle everything, but do I need to do more for array constructors? In typecheck_arrayconstructor_to_array() (from ncnv.pas) you can see I made a template which mimics how dynamic arrays are converted but I think this increased the ref count too high when compared to doing nothing and allowing fpc_copy_proc to handle it.

Thank you, please advise and I'll finish the patch.
Additional InformationAn example:

var
  a: array[0..2] of integer;
begin
  a := [1,2,3];
end.
TagsNo tags attached.
Fixed in Revision46891
FPCOldBugId
FPCTarget-
Attached Files

Activities

Ryan Joseph

2020-04-13 10:05

reporter  

patch_draft.diff (7,026 bytes)   
diff --git a/compiler/defcmp.pas b/compiler/defcmp.pas
index 67f3119557..75887e0bcd 100644
--- a/compiler/defcmp.pas
+++ b/compiler/defcmp.pas
@@ -104,7 +104,8 @@ interface
           tc_variant_2_interface,
           tc_array_2_dynarray,
           tc_elem_2_openarray,
-          tc_arrayconstructor_2_dynarray
+          tc_arrayconstructor_2_dynarray,
+          tc_arrayconstructor_2_array
        );
 
     function compare_defs_ext(def_from,def_to : tdef;
@@ -1146,6 +1147,15 @@ implementation
                               eq:=te_convert_l1;
                               doconv:=tc_string_2_chararray;
                             end
+                        else
+                         { to normal array }
+                         if is_normal_array(def_to) and is_array_constructor(def_from) then
+                           begin
+                             writeln('compare_defs: array constructor to normal array');
+                             // todo: what do we need to check for here?
+                            eq:=te_convert_l2;
+                            doconv:=tc_arrayconstructor_2_array;
+                           end
                         else
                          { other arrays }
                           begin
diff --git a/compiler/ncnv.pas b/compiler/ncnv.pas
index 34f33cbc5e..9544c55937 100644
--- a/compiler/ncnv.pas
+++ b/compiler/ncnv.pas
@@ -119,6 +119,7 @@ interface
           function typecheck_array_2_dynarray : tnode; virtual;
           function typecheck_elem_2_openarray : tnode; virtual;
           function typecheck_arrayconstructor_to_dynarray : tnode; virtual;
+          function typecheck_arrayconstructor_to_array : tnode; virtual;
        private
           function _typecheck_int_to_int : tnode;
           function _typecheck_cord_to_pointer : tnode;
@@ -151,6 +152,7 @@ interface
           function _typecheck_array_2_dynarray : tnode;
           function _typecheck_elem_2_openarray : tnode;
           function _typecheck_arrayconstructor_to_dynarray: tnode;
+          function _typecheck_arrayconstructor_to_array: tnode;
        protected
           function first_int_to_int : tnode;virtual;
           function first_cstring_to_pchar : tnode;virtual;
@@ -2078,6 +2080,77 @@ implementation
       end;
 
 
+    function ttypeconvnode.typecheck_arrayconstructor_to_array : tnode;
+      var
+        newstatement,assstatement:tstatementnode;
+        arrnode:ttempcreatenode;
+        temp2:ttempcreatenode;
+        assnode:tnode;
+        paracount:integer;
+        elemnode:tarrayconstructornode;
+      begin
+        // todo: do I even need this? fpc_copy_proc gets called on the assignment node
+        // so what do I get for doing these extra steps?
+        writeln('convert dynarray to static');
+
+        { assignment of []? }
+        if (left.nodetype=arrayconstructorn) and not assigned(tarrayconstructornode(left).left) then
+          begin
+            result:=cnilnode.create;
+            exit;
+          end;
+
+        if resultdef.typ<>arraydef then
+          internalerror(2017050102);
+
+        tarrayconstructornode(left).force_type(tarraydef(resultdef).elementdef);
+
+        result:=internalstatements(newstatement);
+        { create temp for result }
+        arrnode:=ctempcreatenode.create(totypedef,totypedef.size,tt_persistent,true);
+        addstatement(newstatement,arrnode);
+
+        paracount:=0;
+
+        { create an assignment call for each element }
+        assnode:=internalstatements(assstatement);
+        if left.nodetype=arrayconstructorrangen then
+          internalerror(2016021902);
+        elemnode:=tarrayconstructornode(left);
+        while assigned(elemnode) do
+          begin
+            { arr[i] := param_i }
+            if not assigned(elemnode.left) then
+              internalerror(2017050101);
+            addstatement(assstatement,
+              cassignmentnode.create(
+                cvecnode.create(
+                  ctemprefnode.create(arrnode),
+                  cordconstnode.create(paracount,tarraydef(totypedef).rangedef,false)),
+                elemnode.left));
+            elemnode.left:=nil;
+            inc(paracount);
+            elemnode:=tarrayconstructornode(elemnode.right);
+            if assigned(elemnode) and (elemnode.nodetype<>arrayconstructorn) then
+              internalerror(2016021903);
+          end;
+
+        { get temp for array of lengths }
+        temp2:=ctempcreatenode.create_value(sinttype,sinttype.size,tt_persistent,false,cordconstnode.create(paracount,s32inttype,true));
+        addstatement(newstatement,temp2);
+
+        { add assignment statememnts }
+        addstatement(newstatement,ctempdeletenode.create(temp2));
+        addstatement(newstatement,assnode);
+        { the last statement should return the value as
+          location and type, this is done be referencing the
+          temp and converting it first from a persistent temp to
+          normal temp }
+        addstatement(newstatement,ctempdeletenode.create_normal_temp(arrnode));
+        addstatement(newstatement,ctemprefnode.create(arrnode));
+      end;
+
+      
     function ttypeconvnode._typecheck_int_to_int : tnode;
       begin
         result := typecheck_int_to_int;
@@ -2264,6 +2337,12 @@ implementation
       end;
 
 
+    function ttypeconvnode._typecheck_arrayconstructor_to_array : tnode;
+      begin
+        result:=typecheck_arrayconstructor_to_array;
+      end;
+
+
     function ttypeconvnode.target_specific_general_typeconv: boolean;
       begin
         result:=false;
@@ -2388,7 +2467,8 @@ implementation
           { interface_2_variant} @ttypeconvnode._typecheck_variant_to_interface,
           { array_2_dynarray} @ttypeconvnode._typecheck_array_2_dynarray,
           { elem_2_openarray } @ttypeconvnode._typecheck_elem_2_openarray,
-          { arrayconstructor_2_dynarray } @ttypeconvnode._typecheck_arrayconstructor_to_dynarray
+          { arrayconstructor_2_dynarray } @ttypeconvnode._typecheck_arrayconstructor_to_dynarray,
+          { arrayconstructor_2_array } @ttypeconvnode._typecheck_arrayconstructor_to_array
          );
       type
          tprocedureofobject = function : tnode of object;
@@ -3964,6 +4044,7 @@ implementation
            nil,
            nil,
            @ttypeconvnode._first_nothing,
+           @ttypeconvnode._first_nothing,
            @ttypeconvnode._first_nothing
          );
       type
@@ -4244,7 +4325,8 @@ implementation
            @ttypeconvnode._second_nothing,  { interface_2_variant }
            @ttypeconvnode._second_nothing,  { array_2_dynarray }
            @ttypeconvnode._second_elem_to_openarray,  { elem_2_openarray }
-           @ttypeconvnode._second_nothing   { arrayconstructor_2_dynarray }
+           @ttypeconvnode._second_nothing,  { arrayconstructor_2_dynarray }
+           @ttypeconvnode._second_nothing   { arrayconstructor_2_array }
          );
       type
          tprocedureofobject = procedure of object;
patch_draft.diff (7,026 bytes)   

Florian

2020-04-13 10:12

administrator   ~0122109

You have to figure it out how it works :)

Ryan Joseph

2020-04-13 10:49

reporter   ~0122111

very well. :) In that case from my tests using record management operators I don't need to call typecheck_arrayconstructor_to_array and the assignment just works while keeping the ref count balanced. According to this test the ref count is the same regardless of using normal assignments or the array constructor syntax.

{$mode objfpc}
{$modeswitch advancedrecords}

program tstaticarrinit;
uses
  sysutils;

type
  TRec = record
    ref: integer;
    class operator Initialize(var aRec: TRec);
    class operator Finalize(var aRec: TRec);
    class operator AddRef(var aRec: TRec);
    class operator Copy(constref aSrc: TRec; var aDst: TRec);
  end;

class operator TRec.Initialize(var aRec: TRec);
begin
  aRec.ref := 1;
  writeln('  init ', hexstr(@aRec), ' -> ', aRec.ref);
end;

class operator TRec.Finalize(var aRec: TRec);
begin
  aRec.ref -= 1;
  writeln('  finalize ', hexstr(@aRec), ' -> ', aRec.ref);
end;

class operator TRec.AddRef(var aRec: TRec);
begin
  aRec.ref += 1;
  writeln('  addref ', hexstr(@aRec), ' -> ', aRec.ref);
end;

class operator TRec.Copy(constref aSrc: TRec; var aDst: TRec);
begin
  aDst.ref := aSrc.ref + 1;
  writeln('  copy ', hexstr(@aSrc), ' (',aSrc.ref,')', ' to ', hexstr(@aDst),' (',aDst.ref,')');
end;

 var
    r: TRec;
    a: array[0..2] of TRec;
    i: integer;
  begin
    // array constructor
    a := [r,r,r];
    // normal assignments
    for i := 0 to 2 do
      a[i] := r;
  end.


If that's the case then basically nothing needs to be done except for fixing up the code which I added compare_defs_ext. I'll wait to see if you confirm that because it seems too simple. :)

Sven Barth

2020-04-13 15:03

manager   ~0122124

- your typecheck method should gracefully handle the case of the number of elements in the constructor not matching those in the totypedef; yes, the compiler will complain in the assignment of the temp to the final destination, but vecnodes might trigger other, less transparent errors then while the "incompatible type" error would be enough
- the check for an empty array constructor should error out right away (returning an error node), maybe even prevent this in compare_defs_ext already; hmm... thinking about it maybe you should check the size here instead of in the typecheck, this way the creation of the node is prevented right away, the compiler will complain with the right error and you could simply use an internalerror in your typecheck if the lengths of the array constructor and the target def do not match...
- when you copy an existing function with internalerrors please use new, unique dates for them
- what do you mean with "do I even need this"? You first need to create a static array in a temp by assiging the provided elements from the constructor (which is what your typecheck method does after all; which will result in fpc_copy calls for each element) and then the typeconvn is assigned to the loadn with the static array which is really what calls the fpc_copy function on the array (and then the temp will be freed afterwards); so if you didn't mean anything else, then yes you do need this
- please provide tests

Ryan Joseph

2020-04-13 16:10

reporter   ~0122128

I found that if typecheck_arrayconstructor_to_array wasn't called at all then a copy was still made and the ref counts even matched what I got from assigning 2 static arrays (with managed types) together. That's why I was confused if I even needed that. As you noticed I just copied typecheck_arrayconstructor_to_dynarray because I assumed I would need to do something similar. I didn't know what test to make to even confirm if I was getting the intended behavior so I relied on records like seen above.

The question for me then, what happens if I don't call typecheck_arrayconstructor_to_array and let fpc_copy do everything? Maybe if you know a test I could make to verify I'm doing the copy correct.

Ryan Joseph

2020-04-14 04:20

reporter   ~0122140

Minor changes and tests added. I think what I did in typecheck_arrayconstructor_to_array is correct so I'll leave it like that until I can confirm otherwise. compare_defs now fails if the element count doesn't match but I didn't add a custom error message. This is certainly better design because we don't advance beyond this point now. Multidimensional arrays seemed to work automatically according to my test.
patch2.diff (8,970 bytes)   
diff --git a/compiler/defcmp.pas b/compiler/defcmp.pas
index 67f3119557..7cd9b5414a 100644
--- a/compiler/defcmp.pas
+++ b/compiler/defcmp.pas
@@ -104,7 +104,8 @@ interface
           tc_variant_2_interface,
           tc_array_2_dynarray,
           tc_elem_2_openarray,
-          tc_arrayconstructor_2_dynarray
+          tc_arrayconstructor_2_dynarray,
+          tc_arrayconstructor_2_array
        );
 
     function compare_defs_ext(def_from,def_to : tdef;
@@ -1146,6 +1147,17 @@ implementation
                               eq:=te_convert_l1;
                               doconv:=tc_string_2_chararray;
                             end
+                        else
+                         { to normal array }
+                         if is_normal_array(def_to) and is_array_constructor(def_from) then
+                           begin
+                             { element count must match exactly }
+                             if tarraydef(def_to).elecount=tarraydef(def_from).elecount then
+                               begin
+                                 eq:=te_convert_l2;
+                                 doconv:=tc_arrayconstructor_2_array;
+                               end;
+                           end
                         else
                          { other arrays }
                           begin
diff --git a/compiler/ncnv.pas b/compiler/ncnv.pas
index 34f33cbc5e..89681b058c 100644
--- a/compiler/ncnv.pas
+++ b/compiler/ncnv.pas
@@ -119,6 +119,7 @@ interface
           function typecheck_array_2_dynarray : tnode; virtual;
           function typecheck_elem_2_openarray : tnode; virtual;
           function typecheck_arrayconstructor_to_dynarray : tnode; virtual;
+          function typecheck_arrayconstructor_to_array : tnode; virtual;
        private
           function _typecheck_int_to_int : tnode;
           function _typecheck_cord_to_pointer : tnode;
@@ -151,6 +152,7 @@ interface
           function _typecheck_array_2_dynarray : tnode;
           function _typecheck_elem_2_openarray : tnode;
           function _typecheck_arrayconstructor_to_dynarray: tnode;
+          function _typecheck_arrayconstructor_to_array: tnode;
        protected
           function first_int_to_int : tnode;virtual;
           function first_cstring_to_pchar : tnode;virtual;
@@ -2078,6 +2080,63 @@ implementation
       end;
 
 
+    function ttypeconvnode.typecheck_arrayconstructor_to_array : tnode;
+      var
+        newstatement,assstatement:tstatementnode;
+        arrnode:ttempcreatenode;
+        temp2:ttempcreatenode;
+        assnode:tnode;
+        paracount:integer;
+        elemnode:tarrayconstructornode;
+      begin
+        tarrayconstructornode(left).force_type(tarraydef(resultdef).elementdef);
+
+        result:=internalstatements(newstatement);
+        { create temp for result }
+        arrnode:=ctempcreatenode.create(totypedef,totypedef.size,tt_persistent,true);
+        addstatement(newstatement,arrnode);
+
+        paracount:=0;
+
+        { create an assignment call for each element }
+        assnode:=internalstatements(assstatement);
+        if left.nodetype=arrayconstructorrangen then
+          internalerror(2020041402);
+        elemnode:=tarrayconstructornode(left);
+        while assigned(elemnode) do
+          begin
+            { arr[i] := param_i }
+            if not assigned(elemnode.left) then
+              internalerror(2020041403);
+            addstatement(assstatement,
+              cassignmentnode.create(
+                cvecnode.create(
+                  ctemprefnode.create(arrnode),
+                  cordconstnode.create(paracount,tarraydef(totypedef).rangedef,false)),
+                elemnode.left));
+            elemnode.left:=nil;
+            inc(paracount);
+            elemnode:=tarrayconstructornode(elemnode.right);
+            if assigned(elemnode) and (elemnode.nodetype<>arrayconstructorn) then
+              internalerror(2020041404);
+          end;
+
+        { get temp for array of lengths }
+        temp2:=ctempcreatenode.create_value(sinttype,sinttype.size,tt_persistent,false,cordconstnode.create(paracount,s32inttype,true));
+        addstatement(newstatement,temp2);
+
+        { add assignment statememnts }
+        addstatement(newstatement,ctempdeletenode.create(temp2));
+        addstatement(newstatement,assnode);
+        { the last statement should return the value as
+          location and type, this is done be referencing the
+          temp and converting it first from a persistent temp to
+          normal temp }
+        addstatement(newstatement,ctempdeletenode.create_normal_temp(arrnode));
+        addstatement(newstatement,ctemprefnode.create(arrnode));
+      end;
+
+      
     function ttypeconvnode._typecheck_int_to_int : tnode;
       begin
         result := typecheck_int_to_int;
@@ -2264,6 +2323,12 @@ implementation
       end;
 
 
+    function ttypeconvnode._typecheck_arrayconstructor_to_array : tnode;
+      begin
+        result:=typecheck_arrayconstructor_to_array;
+      end;
+
+
     function ttypeconvnode.target_specific_general_typeconv: boolean;
       begin
         result:=false;
@@ -2388,7 +2453,8 @@ implementation
           { interface_2_variant} @ttypeconvnode._typecheck_variant_to_interface,
           { array_2_dynarray} @ttypeconvnode._typecheck_array_2_dynarray,
           { elem_2_openarray } @ttypeconvnode._typecheck_elem_2_openarray,
-          { arrayconstructor_2_dynarray } @ttypeconvnode._typecheck_arrayconstructor_to_dynarray
+          { arrayconstructor_2_dynarray } @ttypeconvnode._typecheck_arrayconstructor_to_dynarray,
+          { arrayconstructor_2_array } @ttypeconvnode._typecheck_arrayconstructor_to_array
          );
       type
          tprocedureofobject = function : tnode of object;
@@ -3964,6 +4030,7 @@ implementation
            nil,
            nil,
            @ttypeconvnode._first_nothing,
+           @ttypeconvnode._first_nothing,
            @ttypeconvnode._first_nothing
          );
       type
@@ -4244,7 +4311,8 @@ implementation
            @ttypeconvnode._second_nothing,  { interface_2_variant }
            @ttypeconvnode._second_nothing,  { array_2_dynarray }
            @ttypeconvnode._second_elem_to_openarray,  { elem_2_openarray }
-           @ttypeconvnode._second_nothing   { arrayconstructor_2_dynarray }
+           @ttypeconvnode._second_nothing,  { arrayconstructor_2_dynarray }
+           @ttypeconvnode._second_nothing   { arrayconstructor_2_array }
          );
       type
          tprocedureofobject = procedure of object;
diff --git a/tests/test/tstaticarrayconstructor1.pp b/tests/test/tstaticarrayconstructor1.pp
new file mode 100644
index 0000000000..769ce9fef0
--- /dev/null
+++ b/tests/test/tstaticarrayconstructor1.pp
@@ -0,0 +1,9 @@
+{$mode objfpc}
+
+program tstaticarrayconstructor1;
+
+var
+  a: array[0..2] of integer;
+begin
+ 	a := [1,2,3];
+end.
\ No newline at end of file
diff --git a/tests/test/tstaticarrayconstructor2.pp b/tests/test/tstaticarrayconstructor2.pp
new file mode 100644
index 0000000000..8ef23d9166
--- /dev/null
+++ b/tests/test/tstaticarrayconstructor2.pp
@@ -0,0 +1,10 @@
+{%FAIL}
+{$mode objfpc}
+
+program tstaticarrayconstructor1;
+
+var
+  a: array[0..2] of integer;
+begin
+ 	a := [1,2];
+end.
\ No newline at end of file
diff --git a/tests/test/tstaticarrayconstructor3.pp b/tests/test/tstaticarrayconstructor3.pp
new file mode 100644
index 0000000000..a8d380b0ab
--- /dev/null
+++ b/tests/test/tstaticarrayconstructor3.pp
@@ -0,0 +1,10 @@
+{%FAIL}
+{$mode objfpc}
+
+program tstaticarrayconstructor3;
+
+var
+  a: array[0..2] of integer;
+begin
+ 	a := [1,2,3,4];
+end.
\ No newline at end of file
diff --git a/tests/test/tstaticarrayconstructor4.pp b/tests/test/tstaticarrayconstructor4.pp
new file mode 100644
index 0000000000..2e627cec37
--- /dev/null
+++ b/tests/test/tstaticarrayconstructor4.pp
@@ -0,0 +1,14 @@
+{$mode objfpc}
+
+program tstaticarrayconstructor4;
+
+var
+  a: array[0..2,0..2] of integer;
+  i, j: integer;
+begin
+ 	a := [[1,2,3],[10,20,30],[100,200,300]];
+
+  for i := 0 to 2 do
+    for j := 0 to 2 do
+      writeln(i,',',j,':',a[i,j]);
+end.
\ No newline at end of file
diff --git a/tests/test/tstaticarrayconstructor5.pp b/tests/test/tstaticarrayconstructor5.pp
new file mode 100644
index 0000000000..aafdd64cdd
--- /dev/null
+++ b/tests/test/tstaticarrayconstructor5.pp
@@ -0,0 +1,10 @@
+{%FAIL}
+{$mode objfpc}
+
+program tstaticarrayconstructor5;
+
+var
+  a: array[0..0] of integer;
+begin
+ 	a := ['a'];
+end.
\ No newline at end of file
diff --git a/tests/test/tstaticarrayconstructor6.pp b/tests/test/tstaticarrayconstructor6.pp
new file mode 100644
index 0000000000..9d16f38cca
--- /dev/null
+++ b/tests/test/tstaticarrayconstructor6.pp
@@ -0,0 +1,10 @@
+{%FAIL}
+{$mode objfpc}
+
+program tstaticarrayconstructor6;
+
+var
+  a: array[0..2] of integer;
+begin
+ 	a := [];
+end.
\ No newline at end of file
patch2.diff (8,970 bytes)   

Ryan Joseph

2020-06-20 07:32

reporter   ~0123479

I hate to bother you all the time since I know you're busy Sven, but can you look at these? Implicit function specialization is sitting around also and risking me forgetting totally where I was with it. Happy to ask another reviewing but it seems like you're the only one, or at least you've been assigned to work on my patches.

Sven Barth

2020-09-18 17:04

manager   ~0125621

I'm sorry for the long wait. I've finally found the time to look at this and to apply it as it works as expected. Of course one might want to improve the error handling in the future (e.g. that instead of "incompatible type "open array of xxx" and "array[a..b] of xxx"" maybe have "incompatible type "array[0..c] of xxx" and "array[a..b] of xxx"", but that's for another time...

Thank you for the patch. Please test and close if okay.

Next stop is - I hope - the inline specialization patch.

Ryan Joseph

2020-09-19 14:13

reporter   ~0125641

Thanks Sven. I just looked and the inline specialization doesn't have a patch, just a bug report and work to do. :)

Sven Barth

2020-09-19 16:19

manager   ~0125642

Then I meant your GitHub branch. It's been a while since I had looked at that issue. ^^'

Issue History

Date Modified Username Field Change
2020-04-13 10:05 Ryan Joseph New Issue
2020-04-13 10:05 Ryan Joseph File Added: patch_draft.diff
2020-04-13 10:12 Florian Note Added: 0122109
2020-04-13 10:49 Ryan Joseph Note Added: 0122111
2020-04-13 15:03 Sven Barth Note Added: 0122124
2020-04-13 16:10 Ryan Joseph Note Added: 0122128
2020-04-14 04:20 Ryan Joseph File Added: patch2.diff
2020-04-14 04:20 Ryan Joseph Note Added: 0122140
2020-06-20 07:32 Ryan Joseph Note Added: 0123479
2020-09-18 16:59 Sven Barth Assigned To => Sven Barth
2020-09-18 16:59 Sven Barth Status new => assigned
2020-09-18 17:04 Sven Barth Status assigned => resolved
2020-09-18 17:04 Sven Barth Resolution open => fixed
2020-09-18 17:04 Sven Barth Fixed in Version => 3.3.1
2020-09-18 17:04 Sven Barth Fixed in Revision => 46891
2020-09-18 17:04 Sven Barth FPCTarget => -
2020-09-18 17:04 Sven Barth Note Added: 0125621
2020-09-19 14:13 Ryan Joseph Note Added: 0125641
2020-09-19 14:13 Ryan Joseph Status resolved => closed
2020-09-19 16:19 Sven Barth Note Added: 0125642