Add base::FindFirstNonASCII [chromium/src : main]
Anthony Vallee-Dubois (Gerrit)
Anthony Vallee-Dubois added 1 comment![]( "Open in Gerrit")
Hi Greg, PTAL!
I tried to keep this function as similar to `DoIsStringASCII` as possible to make it easier to review since it contains a bunch of unchecked pointer manipulation (for performance). Open to suggestions if you don't think that's the right way to go.
Thanks!
Related details
- Greg Thompson
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Greg Thompson (Gerrit)
Greg Thompson voted and added 4 comments![]( "Open in Gerrit")
Votes added by Greg Thompson
| Code-Review | +1 |
4 comments
codepaths and because it's slightly faster in come cases (when the use"some"
while (characters <= UNSAFE_TODO(end - chars_per_word)) {Please fix this WARNING reported by autoreview issue finding: NIT: Same here. Consider `while (static_cast<size_t>(end - characters) >= chars_per_word)` to avoid the potential pointer arithmetic UB.
while (characters <= UNSAFE_TODO(end - batch_count * chars_per_word)) {Please fix this WARNING reported by autoreview issue finding: NIT: `end - batch_count * chars_per_word` calculates a pointer that could fall before the start of the array if the string is very short (`length < batch_count * chars_per_word`). Computing a pointer before the array bounds is undefined behavior in C++, even if it is never dereferenced.
While this is a preexisting pattern copied from `DoIsStringASCII` and likely works in practice, you can easily avoid the UB by checking the remaining length instead:
```cpp
while (static_cast<size_t>(end - characters) >= batch_count * chars_per_word) {
```
EXPECT_EQ(0u, FindFirstNonASCII(std::string_view("")));are these necessary? `std::string_view` can be implicitly constructed from a string literal.
Related details
- Anthony Vallee-Dubois
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Anthony Vallee-Dubois (Gerrit)
Anthony Vallee-Dubois added 4 comments![]( "Open in Gerrit")
codepaths and because it's slightly faster in come cases (when the useAnthony Vallee-Dubois"some"
Done
while (characters <= UNSAFE_TODO(end - chars_per_word)) {Please fix this WARNING reported by autoreview issue finding: NIT: Same here. Consider `while (static_cast<size_t>(end - characters) >= chars_per_word)` to avoid the potential pointer arithmetic UB.
Done
while (characters <= UNSAFE_TODO(end - batch_count * chars_per_word)) {Please fix this WARNING reported by autoreview issue finding: NIT: `end - batch_count * chars_per_word` calculates a pointer that could fall before the start of the array if the string is very short (`length < batch_count * chars_per_word`). Computing a pointer before the array bounds is undefined behavior in C++, even if it is never dereferenced.
While this is a preexisting pattern copied from `DoIsStringASCII` and likely works in practice, you can easily avoid the UB by checking the remaining length instead:
```cpp
while (static_cast<size_t>(end - characters) >= batch_count * chars_per_word) {
```
Good catch! Done. I'm thinking of sending a separate CL for the same fix in IsStringASCII to avoid any potential future issue. Worth it?
EXPECT_EQ(0u, FindFirstNonASCII(std::string_view("")));are these necessary? `std::string_view` can be implicitly constructed from a string literal.
Good catch, removed.
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Anthony Vallee-Dubois (Gerrit)
Anthony Vallee-Dubois voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change with unreviewed changes![]( "Open in Gerrit")
Unreviewed changes
4 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: base/strings/string_util_impl_helpers.h
Insertions: 7, Deletions: 2.
@@ -244,7 +244,10 @@
// Compare the values of CPU word size.
constexpr size_t chars_per_word = sizeof(MachineWord) / sizeof(Char);
constexpr int batch_count = 16;
- while (characters <= UNSAFE_TODO(end - batch_count * chars_per_word)) {
+ // Process batches until the remaining amount of characters is smaller than a
+ // full batch size.
+ while (static_cast<size_t>(end - characters) >=
+ batch_count * chars_per_word) {
const Char* batch_start = characters;
all_char_bits = 0;
for (int i = 0; i < batch_count; ++i) {
@@ -259,7 +262,9 @@
// Process the remaining words.
const Char* remainder_start = characters;
all_char_bits = 0;
- while (characters <= UNSAFE_TODO(end - chars_per_word)) {
+ // Loop until the remaining byte count is lower than the amount of bytes in a
+ // word.
+ while (static_cast<size_t>(end - characters) >= chars_per_word) {
all_char_bits |= *(reinterpret_cast<const MachineWord*>(characters));
UNSAFE_TODO(characters += chars_per_word);
}
```
```
The name of the file: base/strings/string_util_unittest.cc
Insertions: 12, Deletions: 12.
@@ -565,26 +565,26 @@
TEST(StringUtilTest, FindFirstNonASCII) {
// Branch 1: Empty string
- EXPECT_EQ(0u, FindFirstNonASCII(std::string_view("")));
- EXPECT_EQ(0u, FindFirstNonASCII(std::u16string_view(u"")));
+ EXPECT_EQ(0u, FindFirstNonASCII(""));
+ EXPECT_EQ(0u, FindFirstNonASCII(u""));
// Branch 2: All ASCII strings (returns view.length())
- EXPECT_EQ(5u, FindFirstNonASCII(std::string_view("Hello")));
- EXPECT_EQ(12u, FindFirstNonASCII(std::string_view("Hello World!")));
- EXPECT_EQ(11u, FindFirstNonASCII(std::u16string_view(u"Hello World")));
+ EXPECT_EQ(5u, FindFirstNonASCII("Hello"));
+ EXPECT_EQ(12u, FindFirstNonASCII("Hello World!"));
+ EXPECT_EQ(11u, FindFirstNonASCII(u"Hello World"));
std::string long_ascii(200, 'a');
EXPECT_EQ(200u, FindFirstNonASCII(long_ascii));
// Branch 3: Non-ASCII in remainder / trailing bytes (< 1 batch length)
- EXPECT_EQ(0u, FindFirstNonASCII(std::string_view("\x80World")));
- EXPECT_EQ(5u, FindFirstNonASCII(std::string_view("Hello\x80World")));
- EXPECT_EQ(5u, FindFirstNonASCII(std::u16string_view(u"Hello\u0080World")));
+ EXPECT_EQ(0u, FindFirstNonASCII("\x80World"));
+ EXPECT_EQ(5u, FindFirstNonASCII("Hello\x80World"));
+ EXPECT_EQ(5u, FindFirstNonASCII(u"Hello\u0080World"));
// Branch 4: Non-ASCII in alignment prologue
// Align offset so pointer is not aligned to MachineWord boundary.
std::string prologue_str = " \x80Hello";
- EXPECT_EQ(0u, FindFirstNonASCII(std::string_view(prologue_str).substr(3)));
+ EXPECT_EQ(0u, FindFirstNonASCII(prologue_str.substr(3)));
// Branch 5: Non-ASCII inside 16-word batch (length >= 128 bytes for char)
std::string batch_str(200, 'a');
@@ -606,9 +606,9 @@
EXPECT_EQ(129u, FindFirstNonASCII(remainder_str));
#if defined(WCHAR_T_IS_32_BIT)
- EXPECT_EQ(0u, FindFirstNonASCII(std::wstring_view(L"")));
- EXPECT_EQ(11u, FindFirstNonASCII(std::wstring_view(L"Hello World")));
- EXPECT_EQ(5u, FindFirstNonASCII(std::wstring_view(L"Hello\x80World")));
+ EXPECT_EQ(0u, FindFirstNonASCII(L""));
+ EXPECT_EQ(11u, FindFirstNonASCII(L"Hello World"));
+ EXPECT_EQ(5u, FindFirstNonASCII(L"Hello\x80World"));
#endif
}
```
Change information
Add base::FindFirstNonASCII
This function is an almost copy of base::IsStringASCII, except it
returns the index of the first non-ASCII codepoint in the source string.
This function will be helpful to speed up UTF8 -> UTF16 conversions in
base:: and blink::. When converting UTF8, this function can be used to
identify the bounds of ASCII content present at the beginning of a
string and perform a fast widening copy instead of passing this portion
of the string to the actual decoding algorithm.
UTF8 decoding of ResourceBundle strings in base:: is responsible for a
large portion of CPU time at startup (see
https://issues.chromium.org/issues/517185267), and decoding of text
resources is relevant for a large part of resource loading (most of the
web's content is transmitted as UTF8 and a large portion of that content
is ASCII heavy).
base::IsStringASCII could be replaced with FindFirstNonASCII(source) ==
source.length() but is preserved both for eventual A/B testing of some
codepaths and because it's slightly faster in some cases (when the use
case is only about determining if the string is ASCII and the passed
string contains non-ASCII characters).
- M base/strings/string_util.cc
- M base/strings/string_util.h
- M base/strings/string_util_impl_helpers.h
- M base/strings/string_util_unittest.cc
- M base/strings/string_util_win.cc
- M base/strings/string_util_win.h
Code-Review: +1 by Greg Thompson
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |