[Done] PB 6.40 Alpha 1 - Issue with referenced strings which are modified

[STARGÅTE]

When a string is modified by ReplaceString() and #PB_String_InPlace, it is not duplicated and original string is modified as well.

Code: Select all

Procedure.s Modify(String.s)
	ReplaceString(String, "Beta", "Jota", #PB_String_InPlace)
	ProcedureReturn String
EndProcedure

Procedure.s Test(String.s)
	ProcedureReturn String + Modify(String)
EndProcedure

Debug "Should be: BetaJota"
Debug Test("Beta")

Should be: BetaJota
JotaJota

[NicTheQuick]

But isn't that what "in place" means? The help page even says that the return value needs to be ignored when `#PB_String_InPlace` is used.

Edit: Forget what I just said. I would guess that compiler optimizations just optimize out the whole procedure `Test` or something like that?

[Little John]

Slightly modified test code (using PB's x64 version on Windows):

Code: Select all

Procedure.s Modify(String.s)
	ReplaceString(String, "Beta", "Jota", #PB_String_InPlace)
	ProcedureReturn String
EndProcedure

Procedure.s Test(String.s)
   temp.s = Modify(String)
	ProcedureReturn String + temp  ; Line 8
EndProcedure

Debug "Should be: BetaJota"
Debug Test("Beta")

=> Invalid memory access on line 8

[HanPBF]

I ran the code with PB 6.40a1 and got JotaJota.

As strings are pointers to a list of chars, shoudn't it be correct?

Code: Select all

enableExplicit

Procedure.s Modify(String.s)
	ReplaceString(String, "Beta", "Jota", #PB_String_InPlace)
	ProcedureReturn String
EndProcedure

Procedure.s Test(String.s)
	protected cpy.s = "" + String
	
  protected temp.s = Modify(cpy)
  
	ProcedureReturn String + temp  ; Line 8
EndProcedure

Debug "Should be: BetaJota"
Debug Test("Beta") ; now "BetaJota"

[Fred]

It's the only function of the whole PB commandset to allow this and I don't think it's wise to keep it. It was done when computer were slow to gain a bit of speed, but now it doesn't worth it, as it can introduce bad overflow bugs.

[STARGÅTE]

But isn't that what "in place" means? The help page even says that the return value needs to be ignored when `#PB_String_InPlace` is used.

Yes. The procedure Modify() performs an in-place replacement on the local string "String.s", therefore ReplaceString has no return value.
But as the new PB version now passes the string (internally) by-ref by default, instead of by-value, this replacement is also happen in the caller-procedure Test() where the String is passed by-value to the procedure Modify(), because the modification of the string was not tracked.

It's the only function of the whole PB commandset to allow this and I don't think it's wise to keep it. It was done when computer were slow to gain a bit of speed, but now it doesn't worth it, as it can introduce bad overflow bugs.

It is your decision to keep it (and fix it) or to remove #PB_String_InPlace.
However, it is a good example to show how dangerous it could be to pass strings internally by-ref and only copy them on modifications, if the modification is not noticed. Please don't misunderstand me, I like the new method, because it is much faster! But you must ensure that all modifications are securely tracked.

[Fred]

Of course, it must work perfectly.

[Matheos]

It's the only function of the whole PB commandset to allow this and I don't think it's wise to keep it. It was done when computer were slow to gain a bit of speed, but now it doesn't worth it, as it can introduce bad overflow bugs.

I would be disappointed to see #PB_String_InPlace removed from ReplaceString. It has enabled our product to perform well where we needed quickly to substitute one single character for another single character, and never more than a single character. Clearly it must be used wisely.

Code: Select all

ReplaceString(Params.s, #CHX1 , #LF$, #PB_String_InPlace)

[STARGÅTE]

To my understanding, functions like ReplaceString(), UCase(), LCase(), etc., perform in the new coming PB version automatically an in-place replacement, if the replacement length is the same and the assignment is back to the passed variable: str = function(str, ...)

[jacdelad]

Yet, keeping the inplace parameter would preserve compatibility and the option to use it within longer operations without an extra line.

[Fred]

It's the only function of the whole PB commandset to allow this and I don't think it's wise to keep it. It was done when computer were slow to gain a bit of speed, but now it doesn't worth it, as it can introduce bad overflow bugs.

I would be disappointed to see #PB_String_InPlace removed from ReplaceString. It has enabled our product to perform well where we needed quickly to substitute one single character for another single character, and never more than a single character. Clearly it must be used wisely.

Code: Select all

ReplaceString(Params.s, #CHX1 , #LF$, #PB_String_InPlace)

I understand, but if it's really mandatory for one char replace for speed reason, it should be easy to do a small procedure using pointer to do so. Having buffer overflow issue in a 'beginner' function isn't great IMHO.

[Fred]

To my understanding, functions like ReplaceString(), UCase(), LCase(), etc., perform in the new coming PB version automatically an in-place replacement, if the replacement length is the same and the assignment is back to the passed variable: str = function(str, ...)

Would be great if it works like that, but no, the inplace work is only when the input is a temporary string (result of another function or result of a concatenation, for example UCase(ReadString()) or UCase(a$+b$))

[Fred]

Removed #PB_String_InPlace

[Matheos]

I would be disappointed to see #PB_String_InPlace removed from ReplaceString. It has enabled our product to perform well where we needed quickly to substitute one single character for another single character, and never more than a single character. Clearly it must be used wisely.

Code: Select all

ReplaceString(Params.s, #CHX1 , #LF$, #PB_String_InPlace)

I understand, but if it's really mandatory for one char replace for speed reason, it should be easy to do a small procedure using pointer to do so. Having buffer overflow issue in a 'beginner' function isn't great IMHO.

Agreed, but couldn't you validate that the two strings are of equal length when 'in place' is used? From my past experience, a pointer loop is not as fast as #PB_String_InPlace, regardless of how tight we make the loop. I don't think it's going to work for us, frankly, as we need a rapid replacement.

[Fred]

Small loop should be very fast, can you post your code ?