662669 - nsCharSeparatedTokenizer/nsWhiteSpaceSeparatedTokenizer should use RangedPtr

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Comment 1

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Might as well hit them both here.

Comment 2

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

A really ambitious person might even condense all of our tokenizer implementations into one template class.

Comment 3

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

I think this would make a good entry point for someone who wanted to get started hacking in the Mozilla codebase.

The files of interest here are http://mxr.mozilla.org/mozilla-central/source/xpcom/ds/nsWhitespaceTokenizer.h and/or http://mxr.mozilla.org/mozilla-central/source/xpcom/ds/nsCharSeparatedTokenizer.h.  The nextToken() methods (and whatever else needs changing) should be changed to use the pointer class described at http://whereswalden.com/2011/06/07/introducing-mozillarangedptr-h-a-smart-pointer-for-members-of-buffers/.

It would also be cool to unify some of these tokenizer classes by templatizing them over character size, separating char, etc.  That's not really necessary though.

Feel free to send me email if you want help getting started here.

Comment 4

[Andrew Quartey [:drexler]]

(In reply to comment #3)
> I think this would make a good entry point for someone who wanted to get
> started hacking in the Mozilla codebase.
> 
> The files of interest here are
> http://mxr.mozilla.org/mozilla-central/source/xpcom/ds/nsWhitespaceTokenizer.
> h and/or
> http://mxr.mozilla.org/mozilla-central/source/xpcom/ds/
> nsCharSeparatedTokenizer.h.  The nextToken() methods (and whatever else
> needs changing) should be changed to use the pointer class described at
> http://whereswalden.com/2011/06/07/introducing-mozillarangedptr-h-a-smart-
> pointer-for-members-of-buffers/.
> 
> It would also be cool to unify some of these tokenizer classes by
> templatizing them over character size, separating char, etc.  That's not
> really necessary though.
> 
> Feel free to send me email if you want help getting started here.

I am interested in taking this up and currently am almost done with the necessary changes for both header files. Just giving a heads up.

Comment 5

[Andrew Quartey [:drexler]]

Attachment: Use of RangedPtr in place of raw string iterators

Use of RangedPtr in Tokenizer classes

Comment 6

[Andrew Quartey [:drexler]]

Attachment: Use of RangedPtr in place of raw string iterators

Comment 7

[Jeff Walden [:Waldo]]

Comment on attachment 550776 [details] [diff] [review]
Use of RangedPtr in place of raw string iterators

Review of attachment 550776 [details] [diff] [review]:
-----------------------------------------------------------------

Some driveby comments and food for thought, looks like this needs at least a couple changes in the patch itself (and possibly separate changes to RangedPtr itself, depending -- Kyle, note the API additions considered below).

::: xpcom/ds/nsCharSeparatedTokenizer.h
@@ +85,2 @@
>          // Skip initial whitespace
> +        while (mCurrent != mEnd && IsWhitespace(*mCurrent.get())) {

RangedPtr has a dereference operator defined on it, so you should use *mCurrent here.  Likewise for other places where you do *ptr.get().  Indeed, this pattern actively *defeats* the checking that RangedPtr does, because get() gives you a raw pointer which won't be ranged-checked when dereferenced.

@@ +119,5 @@
>       * Returns the next token.
>       */
>      const nsDependentSubstring nextToken()
>      {
> +		mozilla::RangedPtr<const PRUnichar> tokenStart = mCurrent,

This looks like tab characters to me.  In Mozilla code we use spaces rather than tabs as much as possible (Makefile syntax that requires it being the only exception I can think of).  This looks like a pervasive concern.

@@ +196,5 @@
>      nsCCharSeparatedTokenizer(const nsCSubstring& aSource,
>                                char aSeparatorChar)
> +        : mSeparatorChar(aSeparatorChar),
> +		  mCurrent(aSource.Data(), aSource.Length()),
> +		  mEnd(aSource.Data() + aSource.Length(), aSource.Data(), aSource.Length())

I see this pattern (get a RangedPtr at start, get a RangedPtr at end) here, and I've been writing it in my own uses recently.  RangedPtr is still pretty new and not obviously complete, so maybe we should add better support for this.  RangedPtr::start(T*, size_t length) and RangedPtr::end(T*, size_t length) maybe, than give you the start and end of the range described by the pointer and length?  Thoughts appreciated.

::: xpcom/ds/nsWhitespaceTokenizer.h
@@ +48,5 @@
> +	nsWhitespaceTokenizer(const nsSubstring& aSource) :
> +	  mCurrent(aSource.Data(), aSource.Length()),
> +	  mEnd(aSource.Data() + aSource.Length(), aSource.Data(), aSource.Length())
> +	  {
> +		  while(mCurrent != mEnd && isWhitespace(*mCurrent.get()))

Space between 'while' and '(', please -- likewise for 'for', 'if', and so on in other Mozilla code.

Comment 8

[Andrew Quartey [:drexler]]

Attachment: Use of RangedPtr in place of raw iterators

Inexplicably presumed the dereference operator was not implemented thus my workaround with get(). In addition, i have adjusted the formatting to meet the guidelines and the necessary code changes.

Comment 9

[Andrew Quartey [:drexler]]

> I see this pattern (get a RangedPtr at start, get a RangedPtr at end) here,
> and I've been writing it in my own uses recently.  RangedPtr is still pretty
> new and not obviously complete, so maybe we should add better support for
> this.  RangedPtr::start(T*, size_t length) and RangedPtr::end(T*, size_t
> length) maybe, than give you the start and end of the range described by the
> pointer and length?  Thoughts appreciated.


I agree that creating an end RangePtr just for the sole purpose of it being a sentinel is a bit unwieldy. My idea is that if RangedPtr had internal flags: mFlag and mLen, per se - where mFlag is set based on the whether a length parameter is used in initializing the object or not and mLen to keep track of the pointer increment/decrement, we could then have another function IsAtEnd() for our original checking purpose.

Comment 10

[Jeff Walden [:Waldo]]

It's not so much that creating for the sole purpose of using as a sentinel is unwieldy.  Whether it's used once, twice, or abundantly isn't so important for my particular concern.  What matters to me is that the phrase to create one is unwieldy and repetitive and is thus easier to mistype, and thereby introduce a bug.  Worse, such a bug might be one you had greater confidence in having avoided, courtesy of RangedPtr's internal assertions.  The problem is the shorthands optimize creation for the start of the range only.  If you have a pointer T* p to the start of a range 8 long, you'd have this simple phrase:

  RangedPtr<T> start(p, 8);

But if you want a pointer to the end of the range, 8 past p, you have to do this:

  RangedPtr<T> end(p + 8, p, 8);

And it's worse when "8" is really something like JS_ARRAY_LENGTH(buf), to get the length of a stack buffer.  (JS_ARRAY_LENGTH(b) hiding |sizeof(b)/sizeof(b[0])|, roughly.)  And then if p is really a more verbose expression, you might end up with something like this:

  jschar buf[UINT32_CHAR_BUFFER_LENGTH];
  RangedPtr<jschar> end(buf + JS_ARRAY_LENGTH(buf), buf, JS_ARRAY_LENGTH(buf));

That's moderately understandable.  But in the process buf is repeated four times, and JS_ARRAY_LENGTH is repeated twice.  It should be possible to reduce that.

Regarding internal flags: keep in mind that RangedPtr attempts to be a near drop-in replacement.  If it has a different profile in optimized builds from a raw pointer as far as ABI is concerned, I'd consider that not meeting its goals.  Keeping internal flags looks to me like it would break that, particularly if as it seems to me you're suggesting using IsAtEnd() rather than comparing explicitly against the sentinel.  Although maybe I'm not tracking some nuance of your idea -- please elaborate if you think I've missed something.

I'll try to get to the request tomorrow.  It wouldn't hurt if you kept khuey looking at this too, tho -- first because I assume he actually knows this code and I don't (and won't pretend to, and might well refer the patch to him for a second review even if I do r+ it), second because the API concern can use the extra feedback, and third because I wasn't originally intending to give thorough feedback (just note some low-hanging fruit) (although I certainly can go beyond that, and will if there's a request specifically directed at me, as is the case now).

Comment 11

[Jeff Walden [:Waldo]]

Comment on attachment 550946 [details] [diff] [review]
Use of RangedPtr in place of raw iterators

Review of attachment 550946 [details] [diff] [review]:
-----------------------------------------------------------------

This is definitely getting there -- thanks for what you've done so far!  What I have are mostly style issues I didn't note in the super-quick glance I gave earlier.  But I did see what looks like a bug to me, so it's not all nitpicks.

Some of these classes appear a bit duplicative, in that they're pretty similar except for the character type being |const PRUnichar| or |const char|.  I bet they could be consolidated a bit with application of some templates.  But one thing at a time, certainly -- a followup bug to do that would be nice.

There's also a part of me that wishes the data members of these classes were listed at the top of their classes.  Then, reading logically, you'd see the relatively minimal structure, then you'd see how it was used immediately beneath that.  But that's another style concern for another bug, definitely -- again followup material (separate from the followup noted previously) would be nice.

::: xpcom/ds/nsCharSeparatedTokenizer.h
@@ +43,1 @@
>  #include "nsDependentSubstring.h"

Add a blank line after the RangedPtr.h #include.

@@ +78,5 @@
>            mLastTokenEndedWithWhitespace(PR_FALSE),
>            mLastTokenEndedWithSeparator(PR_FALSE),
>            mSeparatorChar(aSeparatorChar),
> +          mFlags(aFlags),
> +	  mCurrent(aSource.Data(), aSource.Length()),

Do you have a particular reason why you renamed mIter to mCurrent?  Neither name seems super-great to me, but neither is clearly super-bad.  And your diff would be a bit shorter if you kept the mIter name.

@@ +85,2 @@
>          // Skip initial whitespace
> +        while (mCurrent != mEnd && IsWhitespace(*mCurrent)) {

Just as a general prudential matter, when I'm comparing two pointers (or smart pointers) when iterating through a range, I prefer < or <= to !=.  If something were to go horribly awry such that > actually held, < or <= might minimize the extent of the damage.  In contrast != might enable scribbling over significantly more memory, or wasting a ton of time, or whatever.  So please use < here instead of !=.

@@ +98,3 @@
>                       "Should be at beginning of token if there is one");
>  
> +        return mCurrent != mEnd;

mCurrent < mEnd

@@ +120,5 @@
>       */
>      const nsDependentSubstring nextToken()
>      {
> +        mozilla::RangedPtr<const PRUnichar> tokenStart = mCurrent,
> +			                    tokenEnd   = mEnd;

I don't recall seeing Mozilla code that would align = signs in a multiple-declarator this way.  I think it would be more likely to see two separate declarations:

  mozilla::RangedPtr<const PRUnichar> tokenStart = mCurrent;
  mozilla::RangedPtr<const PRUnichar> tokenEnd = mEnd;  

But more substantively: it looks like you renamed begin to tokenStart and end to tokenEnd.  But where before this code assigned mIter to both, now it assigns mEnd to one of them.  This looks like it usually works: tokenEnd is assigned if the loop below is entered.  But it doesn't always work: specifically, I don't think it works on the example in the class documentation (assuming the separator character is ','):

  "foo,,bar,baz" ->       "foo" "" "bar" "baz"

Unfortunately we probably don't have tests specifically for this class, only for it indirectly.  But indirect tests are only so good at covering API edge cases, and they could easily miss one like this.

If you have time and are willing to learn a little more, it probably would be easy to add some very basic tests for this class, and for the other one, to xpcom/tests/.  See these links for information on how you'd do this and for a reasonably simple example:

https://developer.mozilla.org/en/Compiled-code_automated_tests
http://mxr.mozilla.org/mozilla-central/source/xpcom/tests/TestEncoding.cpp

@@ +127,5 @@
>                       "Should be at beginning of token if there is one");
>  
>          // Search until we hit separator or end (or whitespace, if separator
>          // isn't required -- see clause with 'break' below).
> +        while (mCurrent != mEnd && *mCurrent != mSeparatorChar) {

mCurrent < mEnd

@@ +132,2 @@
>            // Skip to end of current word.
> +          while (mCurrent != mEnd &&

mCurrent < mEnd

@@ +138,4 @@
>  
>            // Skip whitespace after current word.
>            mLastTokenEndedWithWhitespace = PR_FALSE;
> +          while (mCurrent != mEnd && IsWhitespace(*mCurrent)) {

mCurrent < mEnd

@@ +149,5 @@
>            } // (else, we'll keep looping until we hit mEnd or SeparatorChar)
>          }
>  
> +        mLastTokenEndedWithSeparator = (mCurrent != mEnd &&
> +                                        *mCurrent == mSeparatorChar);

This != looks like the right thing to use, in contrast to the others.

@@ +154,2 @@
>          NS_ASSERTION((mFlags & SEPARATOR_OPTIONAL) ||
> +                     (mLastTokenEndedWithSeparator == (mCurrent != mEnd)),

mCurrent < mEnd

@@ +163,2 @@
>  
> +            while (mCurrent != mEnd && IsWhitespace(*mCurrent)) {

mCurrent < mEnd

@@ +195,5 @@
>  public:
>      nsCCharSeparatedTokenizer(const nsCSubstring& aSource,
>                                char aSeparatorChar)
> +        : mSeparatorChar(aSeparatorChar),
> +	  mCurrent(aSource.Data(), aSource.Length()),

Repeat my mIter/mCurrent comment from before.

@@ +201,2 @@
>      {
> +        while (mCurrent != mEnd && isWhitespace(*mCurrent)) {

mCurrent < mEnd

@@ +208,5 @@
>       * Checks if any more tokens are available.
>       */
>      PRBool hasMoreTokens()
>      {
> +        return mCurrent != mEnd;

mCurrent < mEnd

@@ +217,5 @@
>       */
>      const nsDependentCSubstring nextToken()
>      {
> +	mozilla::RangedPtr<const char> tokenStart = mCurrent,
> +	                                 tokenEnd = mCurrent;

Two declarations on different lines.  This one doesn't assign mEnd to tokenEnd, so I don't believe it has the "a,,b" => "a" "" "b" problem the previous bit had.

@@ +222,3 @@
>  
>          // Search until we hit separator or end.
> +        while (mCurrent != mEnd && *mCurrent != mSeparatorChar) {

mCurrent < mEnd

@@ +222,4 @@
>  
>          // Search until we hit separator or end.
> +        while (mCurrent != mEnd && *mCurrent != mSeparatorChar) {
> +          while (mCurrent != mEnd &&

mCurrent < mEnd

@@ +230,2 @@
>  
> +          while (mCurrent != mEnd && isWhitespace(*mCurrent)) {

mCurrent < mEnd

@@ +233,5 @@
>            }
>          }
>  
>          // Skip separator (and any whitespace after it).
> +        if (mCurrent != mEnd) {

mCurrent < mEnd

@@ +240,2 @@
>  
> +            while (mCurrent != mEnd && isWhitespace(*mCurrent)) {

mCurrent < mEnd

::: xpcom/ds/nsWhitespaceTokenizer.h
@@ +43,1 @@
>  #include "nsDependentSubstring.h"

Blank line again.

@@ +46,5 @@
>  {
>  public:
> +    nsWhitespaceTokenizer(const nsSubstring& aSource) :
> +    mCurrent(aSource.Data(), aSource.Length()),
> +    mEnd(aSource.Data() + aSource.Length(), aSource.Data(), aSource.Length())

It looks like this should be this, to be consistent style-wise:

    nsWhitespaceTokenizer(const nsSubstring& aSource)
        : mCurrent(aSource.Data(), aSource.Length()),
          mEnd(aSource.Data() + aSource.Length(), aSource.Data(), aSource.Length())

Again I wonder why the mIter -> mCurrent change.

@@ +51,2 @@
>      {
> +	while (mCurrent != mEnd && isWhitespace(*mCurrent)) {

mCurrent < mEnd

@@ +53,2 @@
>  
> +	    ++mCurrent; //move to first non-whitespace character

This code seems clear enough to me that the comment doesn't really add much.  I'd say don't add it.

@@ +59,5 @@
>       * Checks if any more tokens are available.
>       */
>      PRBool hasMoreTokens()
>      {
> +        return mCurrent != mEnd;

mCurrent < mEnd

@@ +67,5 @@
>       * Returns the next token.
>       */
>      const nsDependentSubstring nextToken()
> +    {	
> +        //mark start & end of token

I wouldn't add this comment.

@@ +69,5 @@
>      const nsDependentSubstring nextToken()
> +    {	
> +        //mark start & end of token
> +	const mozilla::RangedPtr<const PRUnichar> tokenStart = mCurrent;
> +	while (mCurrent != mEnd && !isWhitespace(*mCurrent)) {

mCurrent < mEnd

@@ +75,5 @@
> +        } 
> +		
> +	const mozilla::RangedPtr<const PRUnichar> tokenEnd = mCurrent;
> +
> +	//mark start of next token (if possible)

Nor would I add this comment.

@@ +76,5 @@
> +		
> +	const mozilla::RangedPtr<const PRUnichar> tokenEnd = mCurrent;
> +
> +	//mark start of next token (if possible)
> +        while (mCurrent != mEnd && isWhitespace(*mCurrent)) {

mCurrent < mEnd

@@ +99,5 @@
>  };
>  
> +
> +
> +

Why add this whitespace?  I'm not intrinsically opposed to it, but I don't recall seeing a four-blank-line separator used in Mozilla before.

@@ +105,5 @@
>  {
>  public:
> +    nsCWhitespaceTokenizer(const nsCSubstring& aSource) :
> +    mCurrent(aSource.Data(), aSource.Length()),
> +    mEnd(aSource.Data() + aSource.Length(), aSource.Data(), aSource.Length())

Same indentation changes as noted for an earlier class.

@@ +110,2 @@
>      {
> +        while (mCurrent != mEnd && isWhitespace(*mCurrent)) {

mCurrent < mEnd

@@ +118,5 @@
>       * Checks if any more tokens are available.
>       */
>      PRBool hasMoreTokens()
>      {
> +        return mCurrent != mEnd;

mCurrent < mEnd

@@ +126,5 @@
>       * Returns the next token.
>       */
>      const nsDependentCSubstring nextToken()
>      {
> +        //mark start & end of token

I wouldn't add this comment either.

@@ +128,5 @@
>      const nsDependentCSubstring nextToken()
>      {
> +        //mark start & end of token
> +        const mozilla::RangedPtr<const char> tokenStart = mCurrent;
> +        while (mCurrent != mEnd && !isWhitespace(*mCurrent)) {

mCurrent < mEnd

@@ +134,5 @@
>          }
> +	
> +        const mozilla::RangedPtr<const char> tokenEnd = mCurrent;
> +
> +        //mark start of next token (if possible)

Wouldn't add this comment either.

@@ +135,5 @@
> +	
> +        const mozilla::RangedPtr<const char> tokenEnd = mCurrent;
> +
> +        //mark start of next token (if possible)
> +        while (mCurrent != mEnd && isWhitespace(*mCurrent)) {

mCurrent < mEnd

Comment 12

[Andrew Quartey [:drexler]]

Thanks for review and sorry for the tardy reply. First of all the reason for my renaming mIter to mCurrent is out of force of habit. In projects i have undertaken, any pointer that is "moved" is named along the lines of "curr" so thought to do so. tokenEnd and tokenStart was just to provide more obvious names to the cursory reviewer than the ones they replace. With regards to the other styling issues, especially whitespaces, i am not sure whether this is the right place to ask about environment set up but will do so anyway. Currently, i edit the files in Visual Studio via VisualHg & TortoiseHg to keep track of my changes and this is naturally introduces tabs and whitespaces although i later try to remove them using vim. Any suggestions on an alternative setup would be most welcome. 

The nature of the classes are certainly duplicative. One workaround that i thought could be implemented would be to make a whitespace the default delimiter in the CharSeparatedTokenizers if a character isn't specified. However, i thought it prudent to ensure that i had used the RangedPtrs correctly throughout the code before making wholesale changes. At this point, should i focus on the current approach or try to implement the workaround and its associated test ?

Comment 13

[Jeff Walden [:Waldo]]

(In reply to andrew (:drexler) from comment #12)
> However, i thought it prudent to ensure that i had used the RangedPtrs
> correctly throughout the code before making wholesale changes.

Absolutely.  If I gave any indication that you should make particular structural changes while doing the swap here, I didn't mean to do so.

> At this point, should i focus on the current approach or try to implement the
> workaround and its associated test ?

Just the current approach.  Although as I see it, there's never a bad time to add a test.  :-)

Comment 14

[Michael Aro]

Kyle: Is it okay if start work on this task? Please advise.

Comment 15

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Certainly.  Feel free to contact me via email if you'd like guidance.

Comment 16

[Mike Hoye [:mhoye]]

Can you confirm that you're still working on this bug?

Comment 17

[Liz Henry (:lizzard) (relman/hg->git project)]

Kyle, I'm clearing the assignee for this bug since there hasn't been any activity for over a year. Is this still a good mentor bug and [good first bug]?  Thanks!

Comment 18

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

yes.

Comment 19

[Sahil Chelaramani]

Attachment: Made changes using the comments stream to correct previous patch

Hi, I'm new here. I used the comments, to correct the previous patch. Please let me know if anything else is to be changed!

Comment 20

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 788341 [details] [diff] [review]
Made changes using the comments stream to correct previous patch

Review of attachment 788341 [details] [diff] [review]:
-----------------------------------------------------------------

302 Waldo

Comment 21

[Jeff Walden [:Waldo]]

Comment on attachment 788341 [details] [diff] [review]
Made changes using the comments stream to correct previous patch

Review of attachment 788341 [details] [diff] [review]:
-----------------------------------------------------------------

Looks great!  A few style nits, tho, to ensure our code is reasonably consistent to enhance readability, tho.  Post a new patch with these minor fixes, and I'll be happy to land this (probably no earlier than Tuesday, as I'll be out for a weekend trip tomorrow through Monday).

::: xpcom/ds/nsCharSeparatedTokenizer.h
@@ +52,2 @@
>      {
> +        

Get rid of the blank line.

@@ +88,5 @@
>       * Returns the next token.
>       */
>      const nsDependentSubstring nextToken()
>      {
> +        mozilla::RangedPtr<const PRUnichar> tokenStart = mIter, tokenEnd   = mIter;

You should only have one space between |tokenEnd| and the |=| following it.

::: xpcom/ds/nsWhitespaceTokenizer.h
@@ +13,5 @@
>  {
>  public:
> +    nsWhitespaceTokenizer(const nsSubstring& aSource):
> +    mIter(aSource.Data(), aSource.Length()),
> +    mEnd(aSource.Data() + aSource.Length(), aSource.Data(), aSource.Length())

It looks like the normal style in these parts would be

    nsWhitespaceTokenizer(const nsSubstring& aSource)
        : mIter(aSource.Data(), aSource.Length()),
          mEnd(aSource.Data() + aSource.Length(), aSource.Data(), aSource.Length())

@@ +61,5 @@
>  {
>  public:
> +    nsCWhitespaceTokenizer(const nsCSubstring& aSource):
> +    mIter(aSource.Data(), aSource.Length()),
> +    mEnd(aSource.Data() + aSource.Length(), aSource.Data(), aSource.Length())

Same style issue here.

Comment 22

[Sahil Chelaramani]

Attachment: Final changes for the patch.

Made the final changes as suggested. Please review it Khuey

Comment 23

[Sahil Chelaramani]

Attachment: Final changes for the patch

Changed to r=Waldo

Comment 24

[Jeff Walden [:Waldo]]

Comment on attachment 790917 [details] [diff] [review]
Final changes for the patch

Review of attachment 790917 [details] [diff] [review]:
-----------------------------------------------------------------

::: xpcom/ds/nsWhitespaceTokenizer.h
@@ +14,5 @@
>  public:
> +    nsWhitespaceTokenizer(const nsSubstring& aSource):
> +    mIter(aSource.Data(), aSource.Length()),
> +    mEnd(aSource.Data() + aSource.Length(), aSource.Data(),
> +aSource.Length())

Ugh, I think Bugzilla's comment-wrapping might have led you astray here.  The whole mEnd initialization should be on one line, not two.

@@ +63,5 @@
>  public:
> +    nsCWhitespaceTokenizer(const nsCSubstring& aSource):
> +    mIter(aSource.Data(), aSource.Length()),
> +    mEnd(aSource.Data() + aSource.Length(), aSource.Data(),
> +aSource.Length())

Same here, too.

Comment 25

[Sahil Chelaramani]

Attachment: Final changes for the patch;r=Waldo

Comment 26

[Jeff Walden [:Waldo]]

Attachment: Final patch for checkin

On looking at this I noticed a couple other things I hadn't already mentioned, so I just grabbed the patch and made the changes myself.  :-)

It turns out we *don't* actually want mEnd all on one line, because then the line length exceeds 80ch.  At that point, overflowing arguments should move onto a new line, indented so they fit neatly under the first argument passed to construct mEnd.

One other little tweak: I changed a few tab characters in your patch into spaces.  Generally, unless the file you're changing requires it (Makefiles are the most common exception), never use tabs.  :-)  They display differently in different editors, and they mostly make a mess of things.

This should be good to land.

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/35afcb18f192

Comment 28

[Ed Morley [:emorley]]

Backed out for build failures:
https://tbpl.mozilla.org/php/getParsedLog.php?id=26636427&tree=Mozilla-Inbound

https://tbpl.mozilla.org/php/getParsedLog.php?id=26636427&tree=Mozilla-Inbound

Comment 29

[Ed Morley [:emorley]]

Sorry, second link was meant to be
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/0415635dba49

Comment 30

[Sahil Chelaramani]

Attachment: Fixes for Build issues

The error shown in the build log was Reorder, so I reordered the initialization. Please review this patch.

Comment 31

[Jeff Walden [:Waldo]]

Comment on attachment 791284 [details] [diff] [review]
Fixes for Build issues

Review of attachment 791284 [details] [diff] [review]:
-----------------------------------------------------------------

I'll fix the nits and push this later today.  Sorry for the delay getting to this review, I've been busy this week since getting back from the weekend.

::: xpcom/ds/nsCharSeparatedTokenizer.h
@@ +144,5 @@
>      PRUnichar mSeparatorChar;
>      uint32_t  mFlags;
> +    mozilla::RangedPtr<const PRUnichar> mIter;
> +    const mozilla::RangedPtr<const PRUnichar> mEnd;
> +    

Trailing whitespace.

This is not particularly obvious, but for alignment reasons, inside structs, it's better to have larger member fields at the start, and smaller ones at the end.  The overall resulting struct can often be stored in a more compact space.  RangedPtr is (in release builds, we don't care about debug builds) pointer-sized.  So these fields should be at the start, and the smaller-sized bool/PRUnichar/uint32_t should be after it.

(Technically, the ordering should be RangedPtr, uint32_t, PRUnichar, bool overall to keep things most compact.  But it's probably more trouble than it's worth to reorder everything right now, so let's just not contribute to the problem.  :-) )

@@ +220,4 @@
>      char mSeparatorChar;
> +    mozilla::RangedPtr<const char> mIter;
> +    const mozilla::RangedPtr<const char> mEnd;
> +    

Same issue, more trailing whitespace.

Comment 32

[Jeff Walden [:Waldo]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/26bb99dc821b

Thanks for the patch!

Comment 33

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/26bb99dc821b