473676 - String API scrub work

Status: RESOLVED FIXED

(Tamarin Graveyard :: Virtual Machine, defect)

Description

[Steven Johnson]

Attachment: Patch

A few more String API tweaks (after further integration with large existing codebase revealed a few weak points):

-- renamed the various String::Equals() methods into equalsLatin1(), equalsUTF8(), and equalsUTF16() methods, to improve code clarity.

-- renamed the Latin1-literal variant of indexOf() to indexLatin1()

-- renamed the Latin1-literal variant of matches() to matchesLatin1()

-- added contains() and containsLatin1() wrapper methods (equivalent to indexOf()<0) to simplify conversion of existing code

-- added default argument values for sustr() and substr() to match AS3 (the second defaults to 0x7fffffff in both cases)

-- fixed a couple of incorrect usages of internConstantStringLatin1() in DebugCLI.cpp (should have been internStringLatin1() as input string wasn't guaranteed to live forever)

Comment 1

[Steven Johnson]

Attachment: Patch #2

Updated patch with a couple more fixes:

A few more String API tweaks (after further integration with large existing codebase revealed a few weak points):

-- renamed the various String::Equals() methods into equalsLatin1(), equalsUTF8(), and equalsUTF16() methods, to improve code clarity.

-- renamed the Latin1-literal variant of indexOf() to indexLatin1()

-- renamed the Latin1-literal variant of matches() to matchesLatin1()

-- added contains() and containsLatin1() wrapper methods (equivalent to indexOf()<0) to simplify conversion of existing code

-- added default argument values for sustr() and substr() to match AS3 (the second defaults to 0x7fffffff in both cases)

-- fixed a couple of incorrect usages of internConstantStringLatin1() in DebugCLI.cpp (should have been internStringLatin1() as input string wasn't guaranteed to live forever)

-- fix bug in AS3 substr() and substring() calls: code to do proper clamping for negative indices (== start from end) had gone missing. Also, the second arg to substr() variants was incorrectly named "end" (should be "count").

-- fix bug in XMLParser: DOCTYPE case was skipping first 8 bytes for return value, but shouldn't have

Comment 2

[Edwin Smith]

for the substr() and substring() fixes, do we need new tests to cover the problems that the existing suite didn't catch?

Comment 3

[Steven Johnson]

yep, and probably for the XMLParser item too

Comment 4

[Michael Daumling]

Comment on attachment 357086 [details] [diff] [review]
Patch #2

It is a matter of taste, but good for APIs to have uniform names :) After all, 3rd party code will need massive corrections anyway, so why not go the whole distance...

IMHO the missing code from the AS# substr call has been moved to StringObject::substr(), so it was not gone - please verify my assumption.

Comment 5

[Steven Johnson]

(In reply to comment #4)
> IMHO the missing code from the AS# substr call has been moved to
> StringObject::substr(), so it was not gone - please verify my assumption.

evidently not :-) specifically, the Clamp calls dealt with negative indices (== from end rather than start of string) which was the bug in question.

Comment 6

[Steven Johnson]

pushed to redux as changeset:   1305:ec4159061b96

Comment 7

[Brent Baker]

I am trying to add a testcase that would tickle the issue that was in the old XMLParser for the DOCTYPE node (left off the first 8 bytes). But it looks like when the DOCTYPE node is discovered absolutely nothing is done with it.

http://hg.mozilla.org/tamarin-redux/file/tip/core/XMLObject.cpp#l158

So I am not sure if it was ever possible to show the bug as it existed before.

Comment 8

[Chris Peyer]

Is there any testmedia that can be created for this work item?

Comment 9

[Lars T Hansen]

(In reply to comment #8)
> Is there any testmedia that can be created for this work item?

The main items appear to be:

  - string.indexOf and string.match now have separate cases for 8-bit and
    16-bit strings, so we want to make sure we have redundant test cases

  - the logic around argument clamping for string.substr was rewritten,
    so if you're paranoid about changes in behavior there you could at least
    construct test cases, but i expect it's OK.