Cleanup nsTString::ToInteger

RESOLVED FIXED in Firefox 59

Status

()

enhancement
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: erahm, Assigned: erahm)

Tracking

unspecified
mozilla59
Points:
---

Firefox Tracking Flags

(firefox59 fixed)

Details

Attachments

(9 attachments)

Assignee

Description

2 years ago
nsTString::ToInteger could use some cleaning up:

1) It has separate but logicially equivalent implementations for 32 and 64 bit integers
2) We attempt to support auto-detection for the radix, but it's never used
3) Various other nits
Assignee

Comment 1

2 years ago
Replaced the duplicated logic with a shared template implementation.
Attachment #8935607 - Flags: review?(n.nethercote)
Assignee

Updated

2 years ago
Assignee: nobody → erahm
Status: NEW → ASSIGNED
Assignee

Comment 2

2 years ago
kAutoDetect is never actually used in our codebase and makes ToInteger rather
convoluted. This removes the logic for it, the constant itself, and the
resulting dead code.
Attachment #8935608 - Flags: review?(n.nethercote)
Assignee

Comment 3

2 years ago
Attachment #8935609 - Flags: review?(n.nethercote)
Assignee

Comment 4

2 years ago
The `haveValue` logic is no longer necessary. If we don't have a value the
result will always be 0.
Attachment #8935611 - Flags: review?(n.nethercote)
Assignee

Comment 5

2 years ago
In theory other radixes can be passed in but we don't actually handle them.
This asserts that the radix is supported and uses our constants instead of
magic numbers.
Attachment #8935612 - Flags: review?(n.nethercote)
Assignee

Comment 6

2 years ago
Comment on attachment 8935608 [details] [diff] [review]
Part 2: Remove kAutoDetect support from ToInteger

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

::: xpcom/string/nsTStringObsolete.cpp
@@ -216,5 @@
>          }
>          else if((('X'==theChar) || ('x'==theChar)) && (!haveValue || result == 0)) {
> -          continue;
> -        }
> -        else if((('#'==theChar) || ('+'==theChar)) && !haveValue) {

This part can never happen in practice. Any leading '#' or '+' would have been skipped. Any trailing '#' or '+' would have 'haveValue == true'.
Attachment #8935607 - Flags: review?(n.nethercote) → review+
Comment on attachment 8935608 [details] [diff] [review]
Part 2: Remove kAutoDetect support from ToInteger

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

::: xpcom/string/nsTString.h
@@ +379,4 @@
>    /**
>     * Perform string to int conversion.
>     * @param   aErrorCode will contain error if one occurs
> +   * @param   aRadix tells us which radix to assume.

"assume" seems like a strange way to describe it. Maybe just "aRadix is the radix"?

@@ +387,4 @@
>    /**
>     * Perform string to 64-bit int conversion.
>     * @param   aErrorCode will contain error if one occurs
> +   * @param   aRadix tells us which radix to assume.

Ditto.

::: xpcom/string/nsTStringObsolete.cpp
@@ +141,5 @@
> +    //     letters (excluding valid hex digits) but never used the result.
> +    //
> +    //     For example if you pass in "Get the number: 10", aRadix = 10 we'd
> +    //     skip the 'G', and then fail to parse "et the number: 10". If aRadix =
> +    //     16 we'd skip the 'G', and parse just 'e' returning 14.

This is a really weird function!
Attachment #8935608 - Flags: review?(n.nethercote) → review+
Attachment #8935609 - Flags: review?(n.nethercote) → review+
Attachment #8935611 - Flags: review?(n.nethercote) → review+
Comment on attachment 8935612 [details] [diff] [review]
Part 5: Enforce support for only radix10 and radix16

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

I half agree with this patch. Adding the assertion is good, but I think kRadix10 and kRadix16 should be eliminated and replaced with 10 and 16 everywhere. It's silly to have named constants when it doesn't make sense to change those constants to other values. r=me with that.
Attachment #8935612 - Flags: review?(n.nethercote) → review+
Another change that would be nice: invert the `if (cp)` and `if (done)` tests and make them do early returns of 0. That would make it more readable.
Assignee

Comment 10

2 years ago
This reduces the indentation by removing the `if(cp)` and `if(done)` blocks
and just returning early. The `if(cp)` was unnecessary as `BeginReading` will
never return nullptr.

Ignoring whitespace this is just:

> erahm@shetland:~/dev/mozilla-unified$ hg diff -w -c.
> diff --git a/xpcom/string/nsTStringObsolete.cpp b/xpcom/string/nsTStringObsolete.cpp
> --- a/xpcom/string/nsTStringObsolete.cpp
> +++ b/xpcom/string/nsTStringObsolete.cpp
> @@ -129,8 +129,6 @@ ToIntegerCommon(const nsTString<T>& aSrc
>    //initial value, override if we find an integer
>    *aErrorCode=NS_ERROR_ILLEGAL_VALUE;
>  
> -  if(cp) {
> -
>      //begin by skipping over leading chars that shouldn't be part of the number...
>  
>      auto endcp=aSrc.EndReading();
> @@ -163,7 +161,10 @@ ToIntegerCommon(const nsTString<T>& aSrc
>        } //switch
>      }
>  
> -    if (done) {
> +  if (!done) {
> +    // No base 16 or base 10 digits were found.
> +    return 0;
> +  }
>  
>        //integer found
>        *aErrorCode = NS_OK;
> @@ -213,8 +214,7 @@ ToIntegerCommon(const nsTString<T>& aSrc
>        } //while
>        if(negate)
>          result=-result;
> -    } //if
> -  }
> +
>    return result.value();
>  }
Attachment #8935833 - Flags: review?(n.nethercote)
Assignee

Comment 12

2 years ago
Attachment #8935865 - Flags: review?(n.nethercote)
Assignee

Comment 13

2 years ago
Final style cleanup:
  - Comment formatting
  - Move variable declarations to where they're used
  - Don't set NS_OK until we finish processing
  - Early exit for error conditions
Attachment #8935866 - Flags: review?(n.nethercote)
Assignee

Comment 14

2 years ago
(In reply to Nicholas Nethercote [:njn] from comment #7)
> Comment on attachment 8935608 [details] [diff] [review]
> Part 2: Remove kAutoDetect support from ToInteger
> 
> Review of attachment 8935608 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: xpcom/string/nsTString.h
> @@ +379,4 @@
> >    /**
> >     * Perform string to int conversion.
> >     * @param   aErrorCode will contain error if one occurs
> > +   * @param   aRadix tells us which radix to assume.
> 
> "assume" seems like a strange way to describe it. Maybe just "aRadix is the
> radix"?

Agreed, cleaned up locally.


> This is a really weird function!

Hah, that's and understatement. It might make sense to have another followup that proposes breaking the existing behavior. I'm not sure I want to commit to that here.
Assignee

Comment 15

2 years ago
(In reply to Nicholas Nethercote [:njn] from comment #8)
> Comment on attachment 8935612 [details] [diff] [review]
> Part 5: Enforce support for only radix10 and radix16
> 
> Review of attachment 8935612 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I half agree with this patch. Adding the assertion is good, but I think
> kRadix10 and kRadix16 should be eliminated and replaced with 10 and 16
> everywhere. It's silly to have named constants when it doesn't make sense to
> change those constants to other values. r=me with that.

Updated to just use '10' and '16'.
Assignee

Comment 16

2 years ago
(In reply to Nicholas Nethercote [:njn] from comment #9)
> Another change that would be nice: invert the `if (cp)` and `if (done)`
> tests and make them do early returns of 0. That would make it more readable.

Oh right I meant to do that anyhow. This is patch 6. Further style patches in 7, 8, 9 followed from that.
Assignee

Comment 17

2 years ago
Comment on attachment 8935864 [details] [diff] [review]
Part 7: Remove duplicated switch logic from ToInteger

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

::: xpcom/string/nsTStringObsolete.cpp
@@ -152,3 @@
>        case '0': case '1': case '2': case '3': case '4':
>        case '5': case '6': case '7': case '8': case '9':
>          done=true;

Context is missing here, but this is the same as the part that's removed:

> done=true;
> break;
Attachment #8935833 - Flags: review?(n.nethercote) → review+
Comment on attachment 8935864 [details] [diff] [review]
Part 7: Remove duplicated switch logic from ToInteger

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

This makes me realize that Mozilla patches are supposed to have 8 lines of context. I have this in my .hgrc file:

> [defaults]
> diff = -p -U 8
> qdiff = -p -U 8
> qnew = -U

See also https://developer.mozilla.org/en-US/docs/Mercurial/Using_Mercurial.
Attachment #8935864 - Flags: review?(n.nethercote) → review+
Comment on attachment 8935865 [details] [diff] [review]
Part 8: clang-format ToInteger

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

I kind of like how the crazy formatting is a strong signal that this is crazy code... but fair enough :)
Attachment #8935865 - Flags: review?(n.nethercote) → review+
Comment on attachment 8935866 [details] [diff] [review]
Part 9: Further cleanup of minor nits in ToInteger

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

::: xpcom/string/nsTStringObsolete.cpp
@@ -154,4 @@
>          break;
>        // clang-format on
>        case '-':
> -        negate = true; // fall through...

+1 for removing this misleading comment.

@@ +196,3 @@
>        return 0;
>      }
>    } // while

Bonus points if you remove the "// while" comments and any remaining "// if" comments.

@@ +197,5 @@
>      }
>    } // while
>  
> +  // Integer found.
> +  *aErrorCode = NS_OK;

Yeah, that's better.
Attachment #8935866 - Flags: review?(n.nethercote) → review+
Assignee

Comment 21

a year ago
(In reply to Nicholas Nethercote [:njn] from comment #18)
> Comment on attachment 8935864 [details] [diff] [review]
> Part 7: Remove duplicated switch logic from ToInteger
> 
> Review of attachment 8935864 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This makes me realize that Mozilla patches are supposed to have 8 lines of
> context. I have this in my .hgrc file:
> 
> > [defaults]
> > diff = -p -U 8
> > qdiff = -p -U 8
> > qnew = -U
> 
> See also https://developer.mozilla.org/en-US/docs/Mercurial/Using_Mercurial.

Huh, I thought |mach mercurial-setup| was supposed to do that.

Comment 22

a year ago
Pushed by erahm@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b03cecf501a1
Part 1: Combine logic for ToInteger impls. r=njn
https://hg.mozilla.org/integration/mozilla-inbound/rev/f6bb4f9d9b51
Part 2: Remove kAutoDetect support from ToInteger. r=njn
https://hg.mozilla.org/integration/mozilla-inbound/rev/3dfe583bf4c3
Part 3: Update error handling in ToInteger. r=njn
https://hg.mozilla.org/integration/mozilla-inbound/rev/b24d0d0ff3c8
Part 4: Remove haveValue logic from ToInteger. r=njn
https://hg.mozilla.org/integration/mozilla-inbound/rev/2423de2699b1
Part 5: Enforce support for only radix of 10 and 16. r=njn
https://hg.mozilla.org/integration/mozilla-inbound/rev/111b8a7cde41
Part 6: Add early returns to ToInteger. r=njn
https://hg.mozilla.org/integration/mozilla-inbound/rev/293bc5e35a4f
Part 7: Remove duplicated switch logic from ToInteger. r=njn
https://hg.mozilla.org/integration/mozilla-inbound/rev/dcd1da3793f1
Part 8: clang-format ToInteger. r=njn
https://hg.mozilla.org/integration/mozilla-inbound/rev/c28dfb7efe2f
Part 9: Further cleanup of minor nits in ToInteger. r=njn
You need to log in before you can comment on or make changes to this bug.