Bug 63541 - TextPosition refactoring: Merge ZeroBasedNumber and OneBasedNumber classes
Summary: TextPosition refactoring: Merge ZeroBasedNumber and OneBasedNumber classes
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore JavaScript (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-06-28 09:56 PDT by Peter Rybin
Modified: 2011-09-19 11:45 PDT (History)
4 users (show)

See Also:


Attachments
Patch (79.00 KB, patch)
2011-09-16 12:36 PDT, Peter Rybin
no flags Details | Formatted Diff | Diff
Patch (79.12 KB, patch)
2011-09-19 09:47 PDT, Peter Rybin
no flags Details | Formatted Diff | Diff
Patch (79.15 KB, patch)
2011-09-19 10:52 PDT, Peter Rybin
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Peter Rybin 2011-06-28 09:56:53 PDT
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.
Comment 1 Peter Rybin 2011-09-16 12:36:21 PDT
Created attachment 107705 [details]
Patch
Comment 2 Peter Rybin 2011-09-16 12:42:56 PDT
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.
Comment 3 Adam Barth 2011-09-16 12:55:59 PDT
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.
Comment 4 Peter Rybin 2011-09-19 09:47:36 PDT
Created attachment 107875 [details]
Patch
Comment 5 Adam Barth 2011-09-19 10:28:52 PDT
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.
Comment 6 Peter Rybin 2011-09-19 10:52:08 PDT
Created attachment 107883 [details]
Patch
Comment 7 WebKit Review Bot 2011-09-19 11:44:55 PDT
Comment on attachment 107883 [details]
Patch

Clearing flags on attachment: 107883

Committed r95449: <http://trac.webkit.org/changeset/95449>
Comment 8 WebKit Review Bot 2011-09-19 11:45:00 PDT
All reviewed patches have been landed.  Closing bug.