Bug 134692 - Formerly working BASIC function to get a copy of an array no longer working. Interoperability and compatibilty affected.
Summary: Formerly working BASIC function to get a copy of an array no longer working. ...
Status: RESOLVED FIXED
Alias: None
Product: LibreOffice
Classification: Unclassified
Component: BASIC (show other bugs)
Version:
(earliest affected)
7.0.0.0.alpha0+
Hardware: All All
: medium normal
Assignee: Mike Kaganski
URL:
Whiteboard: target:7.1.0 target:7.0.0.2 target:7.3.0
Keywords:
Depends on:
Blocks:
 
Reported: 2020-07-09 14:49 UTC by Wolfgang Jäger
Modified: 2021-07-05 16:49 UTC (History)
2 users (show)

See Also:
Crash report or crash signature:


Attachments
Allowing to run code as proof for the reported bug. (18.62 KB, application/vnd.oasis.opendocument.spreadsheet)
2020-07-09 14:49 UTC, Wolfgang Jäger
Details
Replacement for the first attachment where sole linjes of code were missing (18.75 KB, application/vnd.oasis.opendocument.spreadsheet)
2020-07-09 15:06 UTC, Wolfgang Jäger
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Wolfgang Jäger 2020-07-09 14:49:41 UTC
Created attachment 162852 [details]
Allowing to run code as proof for the reported bug.

It's well known that arrays are assigned by reference generally, and that the ByVal clause is disregarded for arrays passed to functions or subs. (Or, in other words probably that the pointer is treated as the value.) 

However, up to Version 6.4.4.2 (at least):
If inside the body of a function having taken an array as parameter, it was assigned to a new variable, say newVar, and this different variable was treated by 
 
Redim Preseve newVar(without changing the dimensions), 

then newVar could be returned by the function to the caller, and this way created a real copy of the original array passed as parameter.  

As I was told, this should also be the behavior of VBA. 

This doesn' work any longer. 

Leaving it the new way would also introduce logically inacceptable results as demonstrated in the attachment, but mainly it would break existing code. 

In addition workarounds needing to assign array elements one by one to a copy are inefficient by a factor of typically about 500 (judged from my testing).

Check my claims by running the code contained in the attachment stepwise as explaied tom the detail in the attachment itself.
Comment 1 Wolfgang Jäger 2020-07-09 15:06:52 UTC
Created attachment 162853 [details]
Replacement for the first attachment where sole linjes of code were missing

Sorry. 
The original attachment got deleted a few lines of code inadvertently. 
Now thea are in the right place again.
Comment 2 Mike Kaganski 2020-07-09 21:50:34 UTC
I feel quite sure that it's my fix to tdf#129256 that caused the problem with redim preserve (but I hadn't bisected actually).

sub tst
dim a(2 to 5)
b = a
redim preserve b(4 to 6)
for i = lbound(b) to ubound(b)
  b(i) = i
next i
for i = lbound(a) to ubound(a)
  s$ = s$ & " a(" & i & ")=" & a(i)
next i
s$ = s$ & chr(10)
for i = lbound(b) to ubound(b)
  s$ = s$ & " b(" & i & ")=" & b(i)
next i
msgbox s$
end sub
Comment 3 Mike Kaganski 2020-07-10 09:23:34 UTC
https://gerrit.libreoffice.org/c/core/+/98481
Comment 4 Commit Notification 2020-07-10 14:52:27 UTC
Mike Kaganski committed a patch related to this issue.
It has been pushed to "master":

https://git.libreoffice.org/core/commit/2333dfd69c2933004834a572c981ca20a0537422

tdf#134692: copy deep when there are multiple references to the old array

It will be available in 7.1.0.

The patch should be included in the daily builds available at
https://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
https://wiki.documentfoundation.org/Testing_Daily_Builds

Affected users are encouraged to test the fix and report feedback.
Comment 5 Commit Notification 2020-07-14 14:52:58 UTC
Mike Kaganski committed a patch related to this issue.
It has been pushed to "libreoffice-7-0":

https://git.libreoffice.org/core/commit/9a917ee5f7ed2e8476e90a03bf996830b9a88db7

tdf#134692: copy deep when there are multiple references to the old array

It will be available in 7.0.0.2.

The patch should be included in the daily builds available at
https://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
https://wiki.documentfoundation.org/Testing_Daily_Builds

Affected users are encouraged to test the fix and report feedback.
Comment 6 Commit Notification 2021-07-05 15:36:49 UTC
Xisco Fauli committed a patch related to this issue.
It has been pushed to "master":

https://git.libreoffice.org/core/commit/c6cc94ea781699b899ad92ca6ceab2e8f59d1606

tdf#134692: basic_macros: Add unittest

It will be available in 7.3.0.

The patch should be included in the daily builds available at
https://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
https://wiki.documentfoundation.org/Testing_Daily_Builds

Affected users are encouraged to test the fix and report feedback.
Comment 7 Wolfgang Jäger 2021-07-05 16:49:41 UTC
(In reply to Commit Notification from comment #6)
> Xisco Fauli committed a patch related to this issue.
> It has been pushed to "master":
> 
> https://git.libreoffice.org/core/commit/
> c6cc94ea781699b899ad92ca6ceab2e8f59d1606
> 
> tdf#134692: basic_macros: Add unittest
> 
> It will be available in 7.3.0.
> ...
> 
> Affected users are encouraged to test the fix and report feedback.

What is the intention behind the new patch?  
In what way were the patches by Mike Kaganski insufficient?
What's the meaning of "Add unittest"?