Copying records with dynamic arrays

[McDoob]

Hello, world.

I'm having trouble with a program I'm trying to write. I have a record called 'Window' that contains data for a window, including a dynamic array for its contents and color information (You'll note that I have commented a static version of the arrays):

Code: Pascal  [Select][+][-]

1. type
2.   XYType = record
3.     X,Y:byte;
4.   end;
5.   ContentType = array of array of string[1];
6.   ContentColorType = array of array of byte;
7.   WindowType = record
8.     DoubleBorder:boolean;
9.     Origin,Size:XYType;
10. //    Content:array[1..80,1..25] of string[1];
11. //    ContentColor:array[1..80,1..25] of byte;
12.     Content:ContentType;
13.     ContentColor:ContentColorType;
14.     Title:string[MaxTitleLength];
15.   end;
16.   WindowListType = record
17.     InUse:boolean;
18.     Window:WindowType
19.   end;
20.  

I have two procedures (rather a function and a procedure) called CreateWindow and DestroyWindow, and they're fighting with each other! I can successfully call CreateWindow and DestroyWindow once, and the next call to CreateWindow causes an access violation...but they work perfectly with the static version of the arrays.

(Be aware, that I have created empty arrays of size 1,1 called Content and ContentColor, and an empty window called NulWindow in the program's initialization)

Code: Pascal  [Select][+][-]

1. function CreateWindow(w:WindowType):byte;
2. // modifies WindowList
3.  
4. var
5.   AvailWindow,counter:byte;
6.  
7. begin
8.   CreateWindow:=0;
9.   w.Content:=copy(Content);
10.   w.ContentColor:=copy(ContentColor);
11.   with w do
12.     begin
13.       SetLength(Content, Size.X, Size.Y);
14.       SetLength(ContentColor, Size.X, Size.Y);
15.     end;
16.   AvailWindow:=0;
17.   counter:=1;
18.   repeat
19.     if not WindowList[counter].InUse then
20.       begin
21.         AvailWindow:=counter;
22.         counter:=MaxWindows+1;
23.       end
24.     else
25.       counter:=counter+1;
26.   until counter=MaxWindows+1;
27.   if AvailWindow=0 then
28.     error('No Availiable Windows')
29.   else
30.     begin
31.       WindowList[AvailWindow].InUse:=true;
32. // POTENTIAL ERROR HERE, WORKS ONCE
33.       WindowList[AvailWindow].Window:=w;
34.       ActiveWindow:=AvailWindow;
35.       RedrawWindows;
36.       CreateWindow:=AvailWindow;
37.     end;
38. end;
39.  
40. procedure DestroyWindow(W:byte);
41. //modifies WindowList
42. var
43.   counter:byte;
44.  
45. begin
46.   if W=0 then
47.     begin
48.       for counter:=1 to MaxWindows do
49.         begin
50.           WindowList[counter].InUse:=false;
51.  // POTENTIAL ERROR HERE
52.           WindowList[counter].Window:=NulWindow;
53. // Possible solution below, Does NOT fix problem...
54.           WindowList[counter].Window.Content:=copy(Content);
55.           WindowList[counter].Window.ContentColor:=copy(ContentColor);
56.         end;
57.       DrawBG;
58.     end
59.   else
60.     begin
61.       if not WindowList[W].InUse then
62.         error('DestroyWindow: Window not InUse');
63.       WindowList[W].InUse:=false;
64. // POTENTIAL ERROR HERE
65.       WindowList[W].Window:=NulWindow;
66.       WindowList[W].Window.Content:=copy(Content);
67.       WindowList[W].Window.ContentColor:=copy(ContentColor);
68.     end;
69.   counter:=MaxWindows;
70.   repeat
71.     if not WindowList[counter].InUse then counter:=counter-1;
72.   until WindowList[counter].InUse or (counter<1);
73.   ActiveWindow:=counter;
74.   RedrawWindows;
75. end;
76.  

I'm aware that I cannot simply use := to copy the contents of a dynamic array, as that would just copy a pointer, not an array. So I've used 'copy' where I think I'm supposed to (with the static arrays, the 'copy' function calls wouldn't be used). However, there are places where I copy the entire record with := (I've put a comment just before each instance) and I think this must be what's causing the access violation.

I've been banging my head against this wall for a couple of days now. I could always go back to the static arrays, but 80x25 arrays for every window, including tiny ones, seems really wasteful, and totally against the grain for my goal of creating a tiny-footprint text-mode windowed UI for use with other projects.

Any advice would be greatly appreciated!
-McDoob

[McDoob]

Yes, I am aware of nCurses, and that I don't really need to do all of this myself. But there's two reasons I don't want to use it: A) no pre-requisites, and B) for the fun of it.

-McD

[McDoob]

I've narrowed it down to the CreateWindow function. I modified DestoyWindow to only assign WindowList[].InUse to false, and not make any changes to WindowList[].Window. The issue persists. I've also tried changing the way CreateWindow copies the window into the list:

Code: Pascal  [Select][+][-]

1. //      WindowList[AvailWindow].Window:=w;
2.  
3.       WindowList[AvailWindow].Window.Origin.X:=w.Origin.X;
4.       WindowList[AvailWindow].Window.Origin.Y:=w.Origin.Y;
5.       WindowList[AvailWindow].Window.Size.X:=w.Size.X;
6.       WindowList[AvailWindow].Window.Size.Y:=w.Size.Y;
7.       WindowList[AvailWindow].Window.Title:=w.Title;
8.       WindowList[AvailWindow].Window.DoubleBorder:=w.DoubleBorder;
9.       WindowList[AvailWindow].Window.Content:=copy(w.Content);
10.       WindowList[AvailWindow].Window.ContentColor:=copy(w.ContentColor);
11.  

In other words, I've manually copied every entry in the Window record, including calls to the copy function for the arrays. Still, the issue persists. I can create ONE window, and destroy it, but I cannot create a second, either before or after destroying the first.

-McDoob

[ASerge]

Code: Pascal  [Select][+][-]

1.   w.Content:=copy(Content);
2.   w.ContentColor:=copy(ContentColor);
3.  

The Copy function performs shallow copying, in this case it only increases the ref counter.

[McDoob]

The Copy function performs shallow copying, in this case it only increases the ref counter.

I'm not sure I understand this. According to https://wiki.freepascal.org/Dynamic_array that is how := works with dynamic arrays, and exactly why Copy should be used instead:

Code: Pascal  [Select][+][-]

1. program dynamicArrayNilDemo(input, output, stdErr);
2. var
3.         foo, bar: array of char;
4. begin
5.         setLength(foo, 1);
6.         foo[0] := 'X';
7.         // copy _reference_, increase reference count
8.         bar := foo;
9.         // foo becomes nil, reference count is decreased
10.         setLength(foo, 0);
11.         writeLn('length(foo) = ', length(foo),
12.                 '; length(bar) = ', length(bar));
13.        
14.         // decrease reference count another time
15.         bar := nil;
16.         writeLn('length(foo) = ', length(foo),
17.                 '; length(bar) = ', length(bar));
18. end.

If they both do the same thing, why bother with the copy function at all? And does that mean I cannot use a dynamic array in this situation?

EDIT:

For now, I've gone back to static arrays so I can continue working on other projects that use this code.

Besides, with modern machines having several GB of RAM, the ~4 KB per window I'm trying to save probably isn't worth the effort. My test program, which populates 32 windows, only uses ~400KB, and 64 windows only brings it up to 650KB. Sure, I won't be able to target ancient DOS machines with their 640KB barrier...but I wasn't really worried about that anyway...
-McD

[ASerge]

If they both do the same thing, why bother with the copy function at all? And does that mean I cannot use a dynamic array in this situation?

It's just a hint. You don't show what you are doing with the global variables (?) Content and ContentColor or WindowList[AvailWindow].Window.ConXXX fields between function calls. Most likely the error is there.

[McDoob]

It's just a hint. You don't show what you are doing with the global variables (?) Content and ContentColor or WindowList[AvailWindow].Window.ConXXX fields between function calls. Most likely the error is there.

Erm...right. So, I suppose I should elaborate on that...

The global variables for Content and ContentColor were initialized to 1,1 empty arrays purely for debugging purposes, and ignored completely except inside the two function calls. However I do have two other procs that add data to (AddContent) and read from (DrawContent) WindowList[].Window.Content/ContentColor.

The commented out section in DrawContent is what I used while working with the dynamic arrays, while the active code is for static.

Code: Pascal  [Select][+][-]

1. procedure DrawContent(w:byte);
2.  
3. var
4.   x,y:byte;
5. //  cx,cy:byte;
6.  
7. begin
8.   x:=1;
9.   y:=1;
10.   with WindowList[w].Window do
11. {    if length(Content)>1 then
12.       for cx:=Low(Content) to High(Content) do
13.         begin
14.           for cy:=Low(Content[cx]) to High(Content[cx]) do
15.             begin
16.               if not (Content[x,y]='') then
17.                 TextOut(Origin.X+x,Origin.Y+y,ContentColor[cx,cy],(ContentColor[cx,cy] shl 4),Content[cx,cy]);
18.               y:=y+1
19.             end;
20.           y:=1;
21.           x:=x+1;
22.         end;
23. }
24.       for y:=1 to Size.Y do
25.           for x:=1 to Size.X do
26.               if not (Content[x,y]='') then
27.                 TextOut(Origin.X+x,Origin.Y+y,ContentColor[x,y],(ContentColor[x,y] shl 4),Content[x,y]);
28.  
29. end;
30.  
31. procedure AddContent(window,x,y,color,bg:byte;text:string);
32. //TODO: text wrapping
33. var
34.   cx:byte;
35.  
36. begin
37.   if not window=ActiveWindow then
38.     error('AddContent to inactive window');
39.   cx:=1;
40.   repeat
41.     WindowList[window].Window.Content[x+cx-1,y]:=text[cx];
42.     WindowList[window].Window.ContentColor[x+cx-1,y]:=color+(bg shl 4);
43.     cx:=cx+1;
44.   until cx=length(text)+1;
45.   DrawContent(window);
46.   UpdateScreen(false);
47. end;
48.  

As best I can recall right now, that's the only other access to WindowList[].Window.Content/ContentColor

--McDoob

[LV]

If you are OK with static arrays, then replace the copy call with a loop to perform a deep copy with dynamic arrays:

Code: Pascal  [Select][+][-]

1. for i := 0 to High(w.Content) do
2.   for j := 0 to High(w.Content[i]) do
3.     WindowList[AvailWindow].Window.Content[i, j] := w.Content[i, j];
4.  
5. for i := 0 to High(w.ContentColor) do
6.   for j := 0 to High(w.ContentColor[i]) do
7.     WindowList[AvailWindow].Window.ContentColor[i, j] := w.ContentColor[i, j];
8.  

[McDoob]

If you are OK with static arrays, then replace the copy call with a loop to perform a deep copy with dynamic arrays:

I added that to CreateWindow, but the test program still crashes during the second call to CreateWindow.

But, as it turns out, the access violation is actually coming from AddContent. Most of the text from the access violation error is written off-screen when the program crashes, but when I used a separate terminal streched entirely across my monitor, I was able to see which procedure was actually causing the violation. Again, this is only happening after the first window is successfully created.

Here it is, in its current iteration:

Code: Pascal  [Select][+][-]

1. procedure AddContent(window,x,y,color,bg:byte;text:string);
2. //TODO: text wrapping
3. var
4.   cx:byte;
5.  
6. begin
7.   if not window=ActiveWindow then
8.     error('AddContent to inactive window');
9.   cx:=0;
10.     repeat
11.       with WindowList[window].Window do
12.         begin
13. // ACCESS VIOLATION HERE
14.          Content[Low(Content)+x+cx,Low(Content[x])+y]:=text[cx+1];
15.           ContentColor[Low(ContentColor)+x+cx,Low(ContentColor[x])+y]:=color+(bg shl 4);
16.         end;
17.       cx:=cx+1;
18.     until cx=length(text);
19.   DrawContent(window);
20.   UpdateScreen(false);
21. end;
22.  

I guess I have more work to do...
-McDoob

[jamie]

Your logic is in error.

Why do you loop on a empty list?

[McDoob]

Your logic is in error.

Why do you loop on a empty list?

Sorry, what? Where have I done that?

If you mean the WindowList, it wouldn't usually be empty, as CreateWindow inserts windows into it. When I loop through and check the InUse variable, I'm specifically checking if it's empty, or I'm searching for the last entry. Or, In DestroyWindow, if the input is 0 then I go through and set every entry to empty.

[jamie]

Declaration for windlist?

[TRon]

The Copy function performs shallow copying, in this case it only increases the ref counter.

I'm not sure I understand this. According to https://wiki.freepascal.org/Dynamic_array that is how := works with dynamic arrays, and exactly why Copy should be used instead:

If they both do the same thing, why bother with the copy function at all? And does that mean I cannot use a dynamic array in this situation?

You can use copy but you seem to forget that your arrays are an array of an array. The array behind each array is not copied and requires a manual copy (or another copy). That is probably why ASerge mentioned the shallowness.

[McDoob]

Declaration for windlist?

I really don't understand what you're trying to ask me. In the program initialization, every entry in WindowList is set to NulWindow, and InUse is set to false.

Code: Pascal  [Select][+][-]

1. procedure InitVars;
2.  
3. var
4.   counter,counter2:byte;
5.  
6. begin
7.   NulWindow.Origin.X:=1;
8.   NulWindow.Origin.Y:=1;
9.   NulWindow.Size.X:=1;
10.   NulWindow.Size.Y:=1;
11.   NulWindow.Title:='';
12.   NulWindow.DoubleBorder:=false;
13.   NulWindow.Content:=nil;  // Not used for static arrays
14.   NulWindow.ContentColor:=nil; // As above
15.   for counter:=1 to MaxWindows do
16.     begin
17.       WindowList[counter].Window:=NulWindow;
18.       WindowList[counter].InUse:=false;
19.     end;
20. {more initializing}
21. end;
22.  

[McDoob]

You can use copy but you seem to forget that your arrays are an array of an array. The array behind each array is not copied and requires a manual copy (or another copy). That is probably why ASerge mentioned the shallowness.

My head is starting to spin trying to keep track of that statement XD

I believe that problem would be solved by using the suggestion from this post, which I have implemented:

If you are OK with static arrays, then replace the copy call with a loop to perform a deep copy with dynamic arrays:

Code: Pascal  [Select][+][-]

1. for i := 0 to High(w.Content) do
2.   for j := 0 to High(w.Content[i]) do
3.     WindowList[AvailWindow].Window.Content[i, j] := w.Content[i, j];
4.  
5. for i := 0 to High(w.ContentColor) do
6.   for j := 0 to High(w.ContentColor[i]) do
7.     WindowList[AvailWindow].Window.ContentColor[i, j] := w.ContentColor[i, j];
8.  

However, the test program is still crashing when attempting to create a second window, after the first. Actually, when attempting to use AddContent on the second window...I've found that CreateWindow actually works fine, as long as I don't attempt to AddContent to it...
-McDoob