1424120 - Cleanup nsTString::ToInteger

Status: RESOLVED FIXED

(Core :: XPCOM, enhancement)

Description

[Eric Rahm [:erahm]]

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

Comment 1

[Eric Rahm [:erahm]]

Attachment: Part 1: Combine logic for ToInteger impls

Replaced the duplicated logic with a shared template implementation.

Comment 2

[Eric Rahm [:erahm]]

Attachment: Part 2: Remove kAutoDetect support from ToInteger

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.

Comment 3

[Eric Rahm [:erahm]]

Attachment: Part 3: Update error handling in ToInteger

Comment 4

[Eric Rahm [:erahm]]

Attachment: Part 4: Remove haveValue logic from ToInteger

The `haveValue` logic is no longer necessary. If we don't have a value the
result will always be 0.

Comment 5

[Eric Rahm [:erahm]]

Attachment: Part 5: Enforce support for only radix10 and radix16

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.

Comment 6

[Eric Rahm [:erahm]]

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'.

Comment 7

[Nicholas Nethercote [inactive]]

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!

Comment 8

[Nicholas Nethercote [inactive]]

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.

Comment 9

[Nicholas Nethercote [inactive]]

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.

Comment 10

[Eric Rahm [:erahm]]

Attachment: Part 6: Add early returns to ToInteger

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();
>  }

Comment 11

[Eric Rahm [:erahm]]

Attachment: Part 7: Remove duplicated switch logic from ToInteger

Comment 12

[Eric Rahm [:erahm]]

Attachment: Part 8: clang-format ToInteger

Comment 13

[Eric Rahm [:erahm]]

Attachment: Part 9: Further cleanup of minor nits in ToInteger

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

Comment 14

[Eric Rahm [:erahm]]

(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.

Comment 15

[Eric Rahm [:erahm]]

(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'.

Comment 16

[Eric Rahm [:erahm]]

(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.

Comment 17

[Eric Rahm [:erahm]]

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;

Comment 18

[Nicholas Nethercote [inactive]]

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.

Comment 19

[Nicholas Nethercote [inactive]]

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 :)

Comment 20

[Nicholas Nethercote [inactive]]

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.

Comment 21

[Eric Rahm [:erahm]]

(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

[Pulsebot]

Pushed by [e-mail]:
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

Comment 23

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/b03cecf501a1
https://hg.mozilla.org/mozilla-central/rev/f6bb4f9d9b51
https://hg.mozilla.org/mozilla-central/rev/3dfe583bf4c3
https://hg.mozilla.org/mozilla-central/rev/b24d0d0ff3c8
https://hg.mozilla.org/mozilla-central/rev/2423de2699b1
https://hg.mozilla.org/mozilla-central/rev/111b8a7cde41
https://hg.mozilla.org/mozilla-central/rev/293bc5e35a4f
https://hg.mozilla.org/mozilla-central/rev/dcd1da3793f1
https://hg.mozilla.org/mozilla-central/rev/c28dfb7efe2f