Closed Bug 1261382 Opened 4 years ago Closed 4 years ago

Add ReadUntilChar method to Tokenizer

Categories

(Core :: XPCOM, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: valentin, Assigned: valentin)

Details

Attachments

(1 file, 2 obsolete files)

No description provided.
MozReview-Commit-ID: 1qFKPFOQmgJ
Attachment #8737231 - Flags: review?(honzab.moz)
Assignee: nobody → valentin.gosu
Status: NEW → ASSIGNED
Comment on attachment 8737231 [details] [diff] [review]
Add ReadUntilChar method to Tokenizer

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

::: xpcom/ds/Tokenizer.cpp
@@ +196,5 @@
> +Tokenizer::ReadUntilChar(const char until, nsACString& result, bool readToEnd)
> +{
> +  nsDependentCSubstring substring;
> +  bool rv = ReadUntilChar(until, substring, readToEnd);
> +  result = substring;

I personally prefer result.Assign()

::: xpcom/ds/Tokenizer.h
@@ +216,5 @@
>  
>    /**
> +   * Returns the string starting at the current position, until the given char is
> +   * found. If readToEnd is true, and the character isn't found, it will return
> +   * the rest of the string.

The comment says nothing about the result value.

I would say that you don't need the readToEnd arg.  Just read and store the result, when you hit the char, return true.  When not, return false, but the result will be filled with all the (at moment of call) remaining input anyway.  Up to the consumer to decide what to do.

Other thing: how this works with Rollback?

@@ +218,5 @@
> +   * Returns the string starting at the current position, until the given char is
> +   * found. If readToEnd is true, and the character isn't found, it will return
> +   * the rest of the string.
> +   */
> +  bool ReadUntilChar(const char until, nsDependentCSubstring& result, bool readToEnd = false);

please keep aArgument notation.

@@ +219,5 @@
> +   * found. If readToEnd is true, and the character isn't found, it will return
> +   * the rest of the string.
> +   */
> +  bool ReadUntilChar(const char until, nsDependentCSubstring& result, bool readToEnd = false);
> +  bool ReadUntilChar(const char until, nsACString& result, bool readToEnd = false);

I'd slightly more prefer to have ReadUntil(Token const& aToken,...).  It gives more flexibility.
Attachment #8737231 - Flags: review?(honzab.moz) → review-
MozReview-Commit-ID: 1qFKPFOQmgJ
Attachment #8737285 - Flags: review?(honzab.moz)
Attachment #8737231 - Attachment is obsolete: true
Comment on attachment 8737285 [details] [diff] [review]
Add ReadUntilChar method to Tokenizer

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

better, but still don't like it.

thanks for submitting patches instead of mozreview, btw!

::: xpcom/ds/Tokenizer.cpp
@@ +216,5 @@
> +  }
> +
> +  Claim(aResult);
> +  mRollback = rollback;
> +  return true;

so why bother with bool result at all?  this is not a good api design, sorry.

::: xpcom/ds/Tokenizer.h
@@ +216,5 @@
>  
>    /**
> +   * Returns a substring starting at the current position, until the given token is
> +   * found. If the token isn't found, it returns the substring between the current
> +   * position and the end of the string.

still nothing said about the return result and nothing said about how rollback behaves.
Attachment #8737285 - Flags: review?(honzab.moz) → feedback+
MozReview-Commit-ID: 1qFKPFOQmgJ
* * *
[mq]: other

MozReview-Commit-ID: 9gyrmzyegLU
Attachment #8737311 - Flags: review?(honzab.moz)
Attachment #8737285 - Attachment is obsolete: true
Comment on attachment 8737311 [details] [diff] [review]
Add ReadUntil method to Tokenizer

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

Thanks!  Please just update the comments with few nits mentioned and land.

::: xpcom/ds/Tokenizer.h
@@ +248,5 @@
>    }
>  
>    /**
>     * Returns the read cursor position back as it was before the last call of any parsing
> +   * method of Tokenizer (Next, Check*, Skip*, ReadUntil) so that the last operation

I think you can write just Read* because it rolls back for any of the Read* variants.

@@ +295,5 @@
> +   * If aToken isn't found aResult is set to the substring between the current
> +   * position and the end of the string.
> +   * If aToken is found, the method returns true. Otherwise it returns false.
> +   *
> +   * It sets mRollback to the current position, so calling Rollback()

Please don't mention implementation details (members).  Just say that Rollback() will unroll the read cursor to the position as it's been before ReadUntil() has been called.
Attachment #8737311 - Flags: review?(honzab.moz) → review+
https://hg.mozilla.org/mozilla-central/rev/bd66b4259ad2
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in before you can comment on or make changes to this bug.