New issue
Advanced search Search tips

Issue 746157 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Aug 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Task



Sign in to add a comment

Cleanup of string-to-number conversion functions

Project Member Reported by tkent@chromium.org, Jul 19 2017

Issue description

* String::ToFooStrict()
 |base| optional argument is rarely used.
 It seems there are only one instance of ToUIntStrict(&ok, 16).
 Introduce HexToUIntStrict(), and remove |base| arguments?

* String::ToFooStrict() and CharactersToFooStrict().
  They skips leading and trailing spaces.  We should check if we need such non-strict behavior.

* String::ToFoo() (not Strict)
 ToInt() and ToUInt(): The number of usage is low. We might be able to replace them with Strict versions.
  ToInt64() and ToUInt64(): They are not used.  We should remove them.

* Returning a parsed value, and |ok| pointer argument
  Returning |ok| value makes callsites simpler.  ParseHTMLInteger() is designed so.

Current typical code:
  bool ok;
  unsigned value = string.ToUIntStrict(&ok);
  if (!ok)
    ... error handling ...
  foo(value);

Updated code:
  unsigned value;
  if (!string.ToUIntStrict(&value))
    ... error handling ...
  foo(value);

  Probably people have different opinion on this. We should discuss this somewhere.

 
Project Member

Comment 1 by bugdroid1@chromium.org, Jul 20 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/edd7611eee5635db077e46b79232dca6dd3f29a7

commit edd7611eee5635db077e46b79232dca6dd3f29a7
Author: Kent Tamura <tkent@chromium.org>
Date: Thu Jul 20 08:30:06 2017

Introduce blink::String::HexToUIntStrict(), and remove |base| arguments.

The default value of |base| is 10, and only one instance with base=16
exists. This CL adds String::HexToUIntStrict(), and remove |base| arguments of
the following integer parsing functions:

- String::ToIntStrict()
- String::ToUIntStrict()
- String::ToInt64Strict()
- String::ToUInt64Strict()
- StringImpl::ToIntStrict()
- StringImpl::ToUIntStrict()
- StringImpl::ToInt64Strict()
- StringImpl::ToUInt64Strict()
- CharactersToIntStrict()
- CharactersToUIntStrict()
- CharactersToInt64Strict()
- CharactersToUInt64Strict()

Bug:  746157 
Change-Id: If269771609e674d9f1baa279c0ed4954a14b7bc6
Reviewed-on: https://chromium-review.googlesource.com/578536
Commit-Queue: Yuta Kitamura <yutak@chromium.org>
Reviewed-by: Yuta Kitamura <yutak@chromium.org>
Cr-Commit-Position: refs/heads/master@{#488167}
[modify] https://crrev.com/edd7611eee5635db077e46b79232dca6dd3f29a7/third_party/WebKit/Source/core/css/parser/CSSTokenizer.cpp
[modify] https://crrev.com/edd7611eee5635db077e46b79232dca6dd3f29a7/third_party/WebKit/Source/platform/wtf/text/StringImpl.cpp
[modify] https://crrev.com/edd7611eee5635db077e46b79232dca6dd3f29a7/third_party/WebKit/Source/platform/wtf/text/StringImpl.h
[modify] https://crrev.com/edd7611eee5635db077e46b79232dca6dd3f29a7/third_party/WebKit/Source/platform/wtf/text/StringToNumber.cpp
[modify] https://crrev.com/edd7611eee5635db077e46b79232dca6dd3f29a7/third_party/WebKit/Source/platform/wtf/text/StringToNumber.h
[modify] https://crrev.com/edd7611eee5635db077e46b79232dca6dd3f29a7/third_party/WebKit/Source/platform/wtf/text/StringToNumberTest.cpp
[modify] https://crrev.com/edd7611eee5635db077e46b79232dca6dd3f29a7/third_party/WebKit/Source/platform/wtf/text/WTFString.cpp
[modify] https://crrev.com/edd7611eee5635db077e46b79232dca6dd3f29a7/third_party/WebKit/Source/platform/wtf/text/WTFString.h

Project Member

Comment 2 by bugdroid1@chromium.org, Jul 20 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/46dc1cac11057c09a527a9b98bdd4ce63e1cf544

commit 46dc1cac11057c09a527a9b98bdd4ce63e1cf544
Author: Kent Tamura <tkent@chromium.org>
Date: Thu Jul 20 22:44:48 2017

Remove |ok| default argument for CharactersToType() functions.

Almost all callsites specify the |ok| argument. We should not use default
argument for rare cases.

Bug:  746157 
Change-Id: I1cbc3491a510666a4269a6c993e0d811c29ecf5c
Reviewed-on: https://chromium-review.googlesource.com/579108
Reviewed-by: Takayoshi Kochi <kochi@chromium.org>
Commit-Queue: Kent Tamura <tkent@chromium.org>
Cr-Commit-Position: refs/heads/master@{#488456}
[modify] https://crrev.com/46dc1cac11057c09a527a9b98bdd4ce63e1cf544/third_party/WebKit/Source/core/html/HTMLFontElement.cpp
[modify] https://crrev.com/46dc1cac11057c09a527a9b98bdd4ce63e1cf544/third_party/WebKit/Source/platform/WebIconSizesParser.cpp
[modify] https://crrev.com/46dc1cac11057c09a527a9b98bdd4ce63e1cf544/third_party/WebKit/Source/platform/wtf/text/StringToNumber.h

Project Member

Comment 3 by bugdroid1@chromium.org, Jul 21 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/40e9e5f0a1a32bea870e6f0271c97c44f12666b8

commit 40e9e5f0a1a32bea870e6f0271c97c44f12666b8
Author: Kent Tamura <tkent@chromium.org>
Date: Fri Jul 21 04:28:03 2017

Make ToIntegralType() behavior configurable.

Introduce NumberParsingOptions, which is a set of parsing behavior flags, and
CharactersToFoo{,Strict}() pass options to ToIntegralType().

This CL has no behavior changes.
This CL is a preparation to merge CharactersToFoo and CharactersToFooStrict.

Bug:  746157 
Change-Id: I34d001bb193928288b99afcd0d531536105ce7e0
Reviewed-on: https://chromium-review.googlesource.com/580170
Reviewed-by: Takayoshi Kochi <kochi@chromium.org>
Commit-Queue: Kent Tamura <tkent@chromium.org>
Cr-Commit-Position: refs/heads/master@{#488581}
[modify] https://crrev.com/40e9e5f0a1a32bea870e6f0271c97c44f12666b8/third_party/WebKit/Source/platform/wtf/text/StringToNumber.cpp
[modify] https://crrev.com/40e9e5f0a1a32bea870e6f0271c97c44f12666b8/third_party/WebKit/Source/platform/wtf/text/StringToNumber.h

Project Member

Comment 4 by bugdroid1@chromium.org, Jul 24 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/2021fa425d595fba3f6f584ae8b74c3f8526b872

commit 2021fa425d595fba3f6f584ae8b74c3f8526b872
Author: Kent Tamura <tkent@chromium.org>
Date: Mon Jul 24 08:42:44 2017

Merge CharactersToFooStrict and CharactersToFoo by adding NumberParsingOptions argument.

Bug:  746157 
Change-Id: I0bf9c2a5e2799ff0fc0ed701c7ec9b656ed7538e
Reviewed-on: https://chromium-review.googlesource.com/579898
Reviewed-by: Takayoshi Kochi <kochi@chromium.org>
Commit-Queue: Kent Tamura <tkent@chromium.org>
Cr-Commit-Position: refs/heads/master@{#488930}
[modify] https://crrev.com/2021fa425d595fba3f6f584ae8b74c3f8526b872/third_party/WebKit/Source/core/frame/csp/SourceListDirective.cpp
[modify] https://crrev.com/2021fa425d595fba3f6f584ae8b74c3f8526b872/third_party/WebKit/Source/core/html/HTMLDimension.cpp
[modify] https://crrev.com/2021fa425d595fba3f6f584ae8b74c3f8526b872/third_party/WebKit/Source/core/html/HTMLFontElement.cpp
[modify] https://crrev.com/2021fa425d595fba3f6f584ae8b74c3f8526b872/third_party/WebKit/Source/core/html/parser/HTMLParserIdioms.cpp
[modify] https://crrev.com/2021fa425d595fba3f6f584ae8b74c3f8526b872/third_party/WebKit/Source/core/html/parser/HTMLSrcsetParser.cpp
[modify] https://crrev.com/2021fa425d595fba3f6f584ae8b74c3f8526b872/third_party/WebKit/Source/core/html/track/vtt/VTTScanner.cpp
[modify] https://crrev.com/2021fa425d595fba3f6f584ae8b74c3f8526b872/third_party/WebKit/Source/platform/WebIconSizesParser.cpp
[modify] https://crrev.com/2021fa425d595fba3f6f584ae8b74c3f8526b872/third_party/WebKit/Source/platform/wtf/text/StringImpl.cpp
[modify] https://crrev.com/2021fa425d595fba3f6f584ae8b74c3f8526b872/third_party/WebKit/Source/platform/wtf/text/StringToNumber.cpp
[modify] https://crrev.com/2021fa425d595fba3f6f584ae8b74c3f8526b872/third_party/WebKit/Source/platform/wtf/text/StringToNumber.h
[modify] https://crrev.com/2021fa425d595fba3f6f584ae8b74c3f8526b872/third_party/WebKit/Source/platform/wtf/text/StringToNumberTest.cpp

Project Member

Comment 5 by bugdroid1@chromium.org, Jul 26 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/590540cf03a46ddd45da8fd8816cb8f6ccea1525

commit 590540cf03a46ddd45da8fd8816cb8f6ccea1525
Author: Kent Tamura <tkent@chromium.org>
Date: Wed Jul 26 09:40:05 2017

Some improvement of StringToNumber.

- Update comments on kStrict and kLoose.
- Add DCHECK to NumberParsingOptions constrcutor.
- Rename NumberParsingState to NumberParsingResult.

This CL has no behavior changes.

Bug:  746157 
Change-Id: I31400913448ec02bdd7b64c67e2c49ad4d7463c0
Reviewed-on: https://chromium-review.googlesource.com/583950
Commit-Queue: Kent Tamura <tkent@chromium.org>
Reviewed-by: Takayoshi Kochi <kochi@chromium.org>
Cr-Commit-Position: refs/heads/master@{#489579}
[modify] https://crrev.com/590540cf03a46ddd45da8fd8816cb8f6ccea1525/third_party/WebKit/Source/core/html/parser/HTMLParserIdioms.cpp
[modify] https://crrev.com/590540cf03a46ddd45da8fd8816cb8f6ccea1525/third_party/WebKit/Source/platform/wtf/text/StringToNumber.cpp
[modify] https://crrev.com/590540cf03a46ddd45da8fd8816cb8f6ccea1525/third_party/WebKit/Source/platform/wtf/text/StringToNumber.h
[modify] https://crrev.com/590540cf03a46ddd45da8fd8816cb8f6ccea1525/third_party/WebKit/Source/platform/wtf/text/StringToNumberTest.cpp

Project Member

Comment 6 by bugdroid1@chromium.org, Jul 27 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/a14c2b238b0c43cf4ae361d2a9a623b2aa17ffd1

commit a14c2b238b0c43cf4ae361d2a9a623b2aa17ffd1
Author: Kent Tamura <tkent@chromium.org>
Date: Thu Jul 27 07:08:55 2017

Merge WTF::StringImpl::ToFoo and ToFooStrict by adding NumberParsingOptions argument.

This reduces the code size.
This CL has no behavior changes.

Bug:  746157 
Change-Id: Ied311c485185f8ce8576a010cae94e51c795663f
Reviewed-on: https://chromium-review.googlesource.com/587511
Reviewed-by: Takayoshi Kochi <kochi@chromium.org>
Commit-Queue: Kent Tamura <tkent@chromium.org>
Cr-Commit-Position: refs/heads/master@{#489873}
[modify] https://crrev.com/a14c2b238b0c43cf4ae361d2a9a623b2aa17ffd1/third_party/WebKit/Source/core/html/HTMLHRElement.cpp
[modify] https://crrev.com/a14c2b238b0c43cf4ae361d2a9a623b2aa17ffd1/third_party/WebKit/Source/platform/wtf/BUILD.gn
[add] https://crrev.com/a14c2b238b0c43cf4ae361d2a9a623b2aa17ffd1/third_party/WebKit/Source/platform/wtf/text/NumberParsingOptions.h
[modify] https://crrev.com/a14c2b238b0c43cf4ae361d2a9a623b2aa17ffd1/third_party/WebKit/Source/platform/wtf/text/StringImpl.cpp
[modify] https://crrev.com/a14c2b238b0c43cf4ae361d2a9a623b2aa17ffd1/third_party/WebKit/Source/platform/wtf/text/StringImpl.h
[modify] https://crrev.com/a14c2b238b0c43cf4ae361d2a9a623b2aa17ffd1/third_party/WebKit/Source/platform/wtf/text/StringToNumber.h
[modify] https://crrev.com/a14c2b238b0c43cf4ae361d2a9a623b2aa17ffd1/third_party/WebKit/Source/platform/wtf/text/WTFString.cpp

Project Member

Comment 7 by bugdroid1@chromium.org, Jul 31 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/850157267adf8a8f03ade0f14e0a5231b68a52bf

commit 850157267adf8a8f03ade0f14e0a5231b68a52bf
Author: Kent Tamura <tkent@chromium.org>
Date: Mon Jul 31 10:33:25 2017

Add tests and comments for WTF::CharactersToDouble() and WTF::CharactersToFloat().

This CL has no behavior changes.

Bug:  746157 
Change-Id: Ia40822c341d827fccebb3968961205a0dd69a188
Reviewed-on: https://chromium-review.googlesource.com/593369
Reviewed-by: Takayoshi Kochi <kochi@chromium.org>
Commit-Queue: Kent Tamura <tkent@chromium.org>
Cr-Commit-Position: refs/heads/master@{#490740}
[modify] https://crrev.com/850157267adf8a8f03ade0f14e0a5231b68a52bf/third_party/WebKit/Source/platform/wtf/text/StringToNumber.h
[modify] https://crrev.com/850157267adf8a8f03ade0f14e0a5231b68a52bf/third_party/WebKit/Source/platform/wtf/text/StringToNumberTest.cpp
[modify] https://crrev.com/850157267adf8a8f03ade0f14e0a5231b68a52bf/third_party/WebKit/Source/platform/wtf/text/WTFString.h

Project Member

Comment 8 by bugdroid1@chromium.org, Aug 3 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/723fedc061a05e54a762f5eec49ccf22a150d4dc

commit 723fedc061a05e54a762f5eec49ccf22a150d4dc
Author: Kent Tamura <tkent@chromium.org>
Date: Thu Aug 03 05:02:57 2017

Add comments about ToFoo() functions to WTFString.h and AtomicString.h.

NOTRY=true

Bug:  746157 
Change-Id: If1a8d3ecfa0101f6a8d432c658f676089830203c
Reviewed-on: https://chromium-review.googlesource.com/599270
Reviewed-by: Takayoshi Kochi <kochi@chromium.org>
Commit-Queue: Kent Tamura <tkent@chromium.org>
Cr-Commit-Position: refs/heads/master@{#491648}
[modify] https://crrev.com/723fedc061a05e54a762f5eec49ccf22a150d4dc/third_party/WebKit/Source/platform/wtf/text/AtomicString.h
[modify] https://crrev.com/723fedc061a05e54a762f5eec49ccf22a150d4dc/third_party/WebKit/Source/platform/wtf/text/WTFString.h

Project Member

Comment 9 by bugdroid1@chromium.org, Aug 10 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/e613a79f4be6a885b11b5f89cce2c25bd7bd9b11

commit e613a79f4be6a885b11b5f89cce2c25bd7bd9b11
Author: Kent Tamura <tkent@chromium.org>
Date: Thu Aug 10 07:03:47 2017

Remove WTF::String::ToInt64() and ToUInt64().

They are not used.

Bug:  746157 
Change-Id: Id22fb1234c8b3df39785463409729ae0238bfb3a
Reviewed-on: https://chromium-review.googlesource.com/608728
Reviewed-by: Takayoshi Kochi <kochi@chromium.org>
Commit-Queue: Kent Tamura <tkent@chromium.org>
Cr-Commit-Position: refs/heads/master@{#493313}
[modify] https://crrev.com/e613a79f4be6a885b11b5f89cce2c25bd7bd9b11/third_party/WebKit/Source/platform/wtf/text/WTFString.cpp
[modify] https://crrev.com/e613a79f4be6a885b11b5f89cce2c25bd7bd9b11/third_party/WebKit/Source/platform/wtf/text/WTFString.h

Comment 10 by tkent@chromium.org, Aug 10 2017

Owner: tkent@chromium.org
Status: Fixed (was: Available)
Closing.

>   Returning |ok| value makes callsites simpler.  ParseHTMLInteger() is designed so.

I didn't do it.

Sign in to add a comment