Indirectly including TextPosition.h and XPathGrammar.h causes compile errors

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Juan C. Montemayor]

When both TextPosition.h and XPathGrammar.h are included a compile-error is caused, since XPathGrammar.h defines a macro called NUMBER and TextPosition has a typedef named NUMBER.

PS. I am not sure JavaScriptCore is the right component for this bug... If it is, then new test cases aren't relevant since no new code was added.

[Juan C. Montemayor]

Created attachment 98611 [details]
Patch

[Juan C. Montemayor]

Created attachment 98612 [details]
Patch

[Alexey Proskuryakov]

We use all-uppercase words for macros, so TextPosition.h is indeed wrong using it as template parameter. A similar problem exists in InspectorValues.cpp.

TextPosition has a ton of problems - from a bad name to a huge number of style errors in its code. Supporting both 1-based and 0-based offsets is a particularly bad idea - and that's why the class is templatized in the first place. It shouldn't have passed review in its current form.

[Adam Barth]

> Supporting both 1-based and 0-based offsets is a particularly bad idea.

I'm not sure we have much of an option in that regard.  The point of TextPosition is for the C++ type checker to help us avoid off-by-one bugs with 1-based and 0-based line numbering systems.  If you see a way to switch to only a 1-based or 0-based line numbering scheme, please write a patch.

(The style issues, of course, should be fixed.)

[Alexey Proskuryakov]

Do we have a real need to have both numbering systems in core code, not just when displaying a position in Web Inspector? That seems unlikely.

C++ type safety only comes into play when assigning from one TextPosition to another, so off by one errors will still be likely when converting to or from integers anyway.

I don't think that I'm supposed write a patch for every design/review failure that I see.

[Adam Barth]

> Do we have a real need to have both numbering systems in core code, not just when displaying a position in Web Inspector? That seems unlikely.

I don't see how to avoid this issue.  If you see how to avoid it, please let the rest of us know.

> C++ type safety only comes into play when assigning from one TextPosition to another, so off by one errors will still be likely when converting to or from integers anyway.

Indeed.  However, these values get passed around a bunch of places and it's helpful to keep track of whether they're one-based or zero-based.

> I don't think that I'm supposed write a patch for every design/review failure that I see.

You're claiming this machinery isn't needed, but I suspect you don't understand the design pressures that lead to its creation.  If you see a way to remove it, I'd be more than happy to review such a ptach.  I don't see how to do it, which is why we have these types.

[Alexey Proskuryakov]

Yes, It's quite possible that I'm missing some design constraints, as I didn't look at call sites. However, fixing this seems incredibly trivial - in every place that creates TextPosition<OneBasedNumber>, subtract one when constructing. And make matching changes when looking at the values.

[Mark Rowe (bdash)]

Comment on attachment 98612 [details]
Patch

This change is fine and addresses a build issue.  If anyone is unhappy with the design of TextPosition then please file a new bug to follow up on that.

[WebKit Review Bot]

Comment on attachment 98612 [details]
Patch

Clearing flags on attachment: 98612

Committed r89875: <http://trac.webkit.org/changeset/89875>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Peter Rybin]

(In reply to comment #7)
> Yes, It's quite possible that I'm missing some design constraints, as I didn't look at call sites. However, fixing this seems incredibly trivial - in every place that creates TextPosition<OneBasedNumber>, subtract one when constructing. And make matching changes when looking at the values.

Alexey,

0 and 1 (as it is stated inside TextPostion.h) was meant as a part of transition phase -- the first step was to keep all integers values intact, only wrap them in annotation types. That would make the change less drastic and more safe. This way all the places that used to work with 1-based numbers now have OneBaseNumber, and the same with 0-based. As you see there has always been some kind of mixture of different number bases. In particular XMLDocumentParserQt.cpp source still contains incorrect 0 -> 1 reinterpertation, that is now revealed and documented and should be fixed.

You are absolutely right -- WebKit doesn't need to keep both types. In fact they are ready to be easily merged together. You just need to literally merge their interfaces, because all methods have prepared names and constructors are hidden (and decide on internals: 0 or 1). Everything should compile and work.

Unfortunately I'm not involved with WebKit so much to start doing this myself, at least doing this alone.

Peter

[Peter Rybin]

As a follow-up.
This bug in XMLDocumentParserQt.cpp may need TextPosition0 and TextPosition1 types separate: it is proven that there is bug somewhere, because we end up reinterpreting type. Is could be much easier to track the problem down while integers are still annotated. At least that's how it seems to me now.

Aside from this merge should be easy. Probably I should prepare it.

Peter


(In reply to comment #11)
> (In reply to comment #7)
> > Yes, It's quite possible that I'm missing some design constraints, as I didn't look at call sites. However, fixing this seems incredibly trivial - in every place that creates TextPosition<OneBasedNumber>, subtract one when constructing. And make matching changes when looking at the values.
> 
> Alexey,
> 
> 0 and 1 (as it is stated inside TextPostion.h) was meant as a part of transition phase -- the first step was to keep all integers values intact, only wrap them in annotation types. That would make the change less drastic and more safe. This way all the places that used to work with 1-based numbers now have OneBaseNumber, and the same with 0-based. As you see there has always been some kind of mixture of different number bases. In particular XMLDocumentParserQt.cpp source still contains incorrect 0 -> 1 reinterpertation, that is now revealed and documented and should be fixed.
> 
> You are absolutely right -- WebKit doesn't need to keep both types. In fact they are ready to be easily merged together. You just need to literally merge their interfaces, because all methods have prepared names and constructors are hidden (and decide on internals: 0 or 1). Everything should compile and work.
> 
> Unfortunately I'm not involved with WebKit so much to start doing this myself, at least doing this alone.
> 
> Peter

[Peter Rybin]

This is how merge could be done. In place of ZeroBasedNumber and OneBasedNumber the following code could be placed:

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;


This should compile. Obviously, this is only a first step of refactoring.
I would be happy to help doing this, but probably I'm not ready to start this whole thing myself.

[Alexey Proskuryakov]

Would you be willing to file a new bug, and to post these ideas there?

[Peter Rybin]

(In reply to comment #14)
> Would you be willing to file a new bug, and to post these ideas there?

Sure.
https://bugs.webkit.org/show_bug.cgi?id=63541