issue-#28 Fixing a bug in Strings.Replace in case of truncat

Post Reply
User avatar
Josef Templ
Posts: 2047
Joined: Tue Sep 17, 2013 6:50 am

issue-#28 Fixing a bug in Strings.Replace in case of truncat

Post by Josef Templ »

This issue is about a very old bug in Strings.Replace. In case of
truncation of the resulting string, the result is not terminated with a 0X character.
The fix is simple and a proposal exists in CPC 1.7 rc5.

Since the truncation did not work until now, it is unlikely that anybody ever used truncation with Strings.Replace.
This means that truncation with Strings.Replace is a meaningless feature and we actually have two options:
1. fix the bug as proposed
2. create a TRAP in case of truncation.

The straightforward way would of course be to fix the bug.
There is one other procedure that does implicit truncation: Strings.Extract.

Any comments?

- Josef
cfbsoftware
Posts: 204
Joined: Wed Sep 18, 2013 10:06 pm
Contact:

Re: issue-#28 Fixing a bug in Strings.Replace in case of tru

Post by cfbsoftware »

The Strings documentation for Replace says "The result is truncated if s is not large enough." so even if it did not work in practice that would have been the expectation. Therefore we should conform to that rather than trapping.
User avatar
Josef Templ
Posts: 2047
Joined: Tue Sep 17, 2013 6:50 am

Re: issue-#28 Fixing a bug in Strings.Replace in case of tru

Post by Josef Templ »

OK, the bug fix is in issue-#28.
see the diff under http://redmine.blackboxframework.org/pr ... 41cd4a314a

There is an obvious potential for a trivial optimization, by the way.
The special case of not changing the length of the result is not treated separately
but leads to shifting the remaining part by zero characters.
See the two lines after the fix (DEC(k);)

- Josef
User avatar
Josef Templ
Posts: 2047
Joined: Tue Sep 17, 2013 6:50 am

Re: issue-#28 Fixing a bug in Strings.Replace in case of tru

Post by Josef Templ »

I did not receive any comments on the proposal of adding a trivial optimization
for the case where there is no change in the length of the string.
There are three cases:
(1) the string length decreases,
(2) the string length is unchanged,
(3) the string length increases.
Shifting the right part of the string by zero characters is not required in (2).
It is of course not strictly a bug fix but it is so simple and obvious that it is almost a bug not to
optimize it.

- Josef
Ivan Denisov
Posts: 1700
Joined: Tue Sep 17, 2013 12:21 am
Location: Russia

Re: issue-#28 Fixing a bug in Strings.Replace in case of tru

Post by Ivan Denisov »

Let's optimize!
cfbsoftware
Posts: 204
Joined: Wed Sep 18, 2013 10:06 pm
Contact:

Re: issue-#28 Fixing a bug in Strings.Replace in case of tru

Post by cfbsoftware »

If you really want to optimise the special case where the string length is unchanged then you should check at the start and process that case separately - the algorithm is much simpler and more efficient than the complex general case that already exists.

However, your time might be better spent completely rewriting Extract and Replace so that the existing contorted code is more comprehensible, verifiable and hence easier to maintain. My first thought on how to do this would be to write two simple, easily verifiable primitives Head (which returns the first n chars of a string) and Tail (which returns the last n chars of a string).

Once those are completed you could then formulate Extract and Replace in terms of both of those and the built-in string concatenation operator.

e.g. All of the source code of the procedure

Code: Select all

Replace (s, pos, len, rep);
simply becomes something like:

Code: Select all

s := Head(s, pos) + rep + Tail(s, LEN(s$) - len - pos);
Extract is even simpler:

Code: Select all

res := Tail(Head(s, pos + len), len);
The unchanged string length optimisation then would be something like:

Code: Select all

IF (len = LEN(rep$)) THEN
   ReplaceChars(s, pos, rep)
ELSE
   ReplaceString(s, pos, len, rep);
Regards,
Chris
User avatar
Josef Templ
Posts: 2047
Joined: Tue Sep 17, 2013 6:50 am

Re: issue-#28 Fixing a bug in Strings.Replace in case of tru

Post by Josef Templ »

Well, the complications of Replace come to a large degree from the automatic truncation semantics not from the copy operations.
In addition, I think a complete rewrite of parts of the module Strings would really need an issue on its own.
So my proposal for a 'small' fix is checked in. Please have a look at the diff
http://redmine.blackboxframework.org/pr ... 3&type=sbs.

Please note that I also optimized the first loop that copies the part of rep that always needs to be copied
by refactoring the common subexpression rep[j] into the variable ch.
This allows for a very cheap test (IF ch # 0X) if there is a need for copying the remaining part further below.

- Josef
User avatar
Josef Templ
Posts: 2047
Joined: Tue Sep 17, 2013 6:50 am

Re: issue-#28 Fixing a bug in Strings.Replace in case of tru

Post by Josef Templ »

are we ready to vote on issue-#28?

- Josef
User avatar
Robert
Posts: 1024
Joined: Sat Sep 28, 2013 11:04 am
Location: Edinburgh, Scotland

Re: issue-#28 Fixing a bug in Strings.Replace in case of tru

Post by Robert »

I'm happy to vote for voting on it!
Post Reply