Bug 63541 – TextPosition refactoring: Merge ZeroBasedNumber and OneBasedNumber classes

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore JavaScript

[Peter Rybin]

JavaScriptCore/wtf/text/TextPosition.h has dual types: [Zero/One]BasedNumber and TextPosition[0/1]. Intended only for transition phase, these dual types now ought to be merged down together.
This is a step 1.

Classes ZeroBasedNumber and OneBasedNumber should be replaced with one class BasedNumber (or maybe "NumberWithBase?").
In source file TextPositions.h old classes should be replaced with the following text:

"
class BasedNumber {
public:
    static BasedNumber fromZeroBasedInt(int zeroBasedInt) { return BasedNumber(zeroBasedInt); }
    static BasedNumber fromOneBasedInt(int oneBasedInt) { return BasedNumber(oneBasedInt - 1); }

    BasedNumber() {}

    int zeroBasedInt() const { return m_value0; }
    int oneBasedInt() const { return m_value0 + 1; }

    int convertAsOneBasedInt() const { return m_value0 + 1; }
    int convertAsZeroBasedInt() const { return m_value0; }

    BasedNumber convertToOneBased() const { return *this; }
    BasedNumber convertToZeroBased() const { return *this; }

    bool operator==(BasedNumber other) { return m_value0 == other.m_value0; }
    bool operator!=(BasedNumber other) { return !((*this) == other); }

    static BasedNumber base() { return BasedNumber(0); }
    static BasedNumber belowBase() { return BasedNumber(-1); }

private:
    BasedNumber(int value0) : m_value0(value0) {}
    int m_value0;
};

typedef BasedNumber ZeroBasedNumber;
typedef BasedNumber OneBasedNumber;
"


P.s. next steps could be:
1. make TextPosition no longer template -- inline its only possible actual parameter BasedNumber;
2. inline such typedefs as "typedef BasedNumber ZeroBasedNumber;" and "typedef TextPosition TextPosition0;" etc throughout source base;
3. inline trivial methods BasedNumber::convertTo[Zero/One]Based.

Methods DocumentParser::lineNumber currently return "int" (0-based), they should better return BasedNumber instead.
Fixing bug in XMLDocumentParserQt  ( https://bugs.webkit.org/show_bug.cgi?id=63540 ) is a good thing to do before these refactorings above.

[Peter Rybin]

Created attachment 107705 [details]
Patch

[Peter Rybin]

I'm not absolutely happy with "BasedNumber" type name, but it was the simplest option.

There are many places across the codebase that could use BasedNumber instead of int, but I'm not sure I can propagate the new type throughout entire WebKit here.

[Adam Barth]

Comment on attachment 107705 [details]
Patch

This change is great.  Maybe instead of BasedNumber we should use the name OrdinalNumber?

http://en.wikipedia.org/wiki/Ordinal_number

That's a number used for ordering as opposed to a number use for counting, which is a cardinal number.

[Peter Rybin]

Created attachment 107875 [details]
Patch

[Adam Barth]

Comment on attachment 107875 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=107875&action=review

> Source/WebCore/ChangeLog:8
> +        No new tests. (OOPS!)

This line will prevent the patch from being landed automatically.  In this case, you should explain that there are no new tests because this patch is not changing behavior.

[Peter Rybin]

Created attachment 107883 [details]
Patch

[WebKit Review Bot]

Comment on attachment 107883 [details]
Patch

Clearing flags on attachment: 107883

Committed r95449: <http://trac.webkit.org/changeset/95449>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.