Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2500723002:
Add UTF-16 string literal constructors to String classes (Closed)
DescriptionAdd UTF-16 string literal constructors to String classes
The patch adds UTF-16 string literal[1] constructors to String,
AtomicString, and StringView.
[1] http://en.cppreference.com/w/cpp/language/string_literal
Committed: https://crrev.com/95c5d59165c76528ee1cff41a2faa804d796ce9c
Cr-Commit-Position: refs/heads/master@{#431827}
Patch Set 1 #
Total comments: 1
Patch Set 2 : esprehn nit (cstr -> chars) #
Total comments: 1
Messages
Total messages: 24 (15 generated)
The CQ bit was checked by kojii@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== string for char16_t BUG= ========== to ========== Add UTF-16 string literal constructors to String/AtomicString/StringView The patch adds UTF-16 string literal[1] constructors to String/ AtomicString/StringView. [1] http://en.cppreference.com/w/cpp/language/string_literal ==========
Description was changed from ========== Add UTF-16 string literal constructors to String/AtomicString/StringView The patch adds UTF-16 string literal[1] constructors to String/ AtomicString/StringView. [1] http://en.cppreference.com/w/cpp/language/string_literal ========== to ========== Add UTF-16 string literal constructors to String classes The patch adds UTF-16 string literal[1] constructors to String, AtomicString, and StringView. [1] http://en.cppreference.com/w/cpp/language/string_literal ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
kojii@chromium.org changed reviewers: + esprehn@chromium.org, haraken@chromium.org
PTAL. Is this ok to add? I used u"utf-16" string literals in my tests and found that I needed reinterpret_cast.
(Elliott should be a better reviewer for the String implementation.)
Lgtm w/ a nit https://codereview.chromium.org/2500723002/diff/1/third_party/WebKit/Source/w... File third_party/WebKit/Source/wtf/text/WTFString.h (right): https://codereview.chromium.org/2500723002/diff/1/third_party/WebKit/Source/w... third_party/WebKit/Source/wtf/text/WTFString.h:87: String(const char16_t* cstr) : String(reinterpret_cast<const UChar*>(cstr)) {} chars
The CQ bit was checked by kojii@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thank you (on your Sunday evening ;)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by kojii@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from esprehn@chromium.org Link to the patchset: https://codereview.chromium.org/2500723002/#ps20001 (title: "esprehn nit (cstr -> chars)")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Add UTF-16 string literal constructors to String classes The patch adds UTF-16 string literal[1] constructors to String, AtomicString, and StringView. [1] http://en.cppreference.com/w/cpp/language/string_literal ========== to ========== Add UTF-16 string literal constructors to String classes The patch adds UTF-16 string literal[1] constructors to String, AtomicString, and StringView. [1] http://en.cppreference.com/w/cpp/language/string_literal Committed: https://crrev.com/95c5d59165c76528ee1cff41a2faa804d796ce9c Cr-Commit-Position: refs/heads/master@{#431827} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/95c5d59165c76528ee1cff41a2faa804d796ce9c Cr-Commit-Position: refs/heads/master@{#431827}
Message was sent while issue was closed.
jshin@chromium.org changed reviewers: + jshin@chromium.org
Message was sent while issue was closed.
While working on https://crbug.com/693046, I came across this CL. Sorry for adding a comment to a closed CL. A goma issue mentioned below can be resolved. There's nothing to worry about. We'll see. https://codereview.chromium.org/2500723002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/wtf/text/WTFString.h (right): https://codereview.chromium.org/2500723002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/wtf/text/WTFString.h:88: : String(reinterpret_cast<const UChar*>(chars)) {} When the C++ spec is strictly interpreted, this casting would lead to an undefined behavior with some optimization mode. https://gcc.gnu.org/onlinedocs/gcc-4.9.2/gcc/Optimize-Options.html. See -fstrict-aliasing. Anyway, this adds an interesting wrinkle to ICU update to 59(to-be), which changes UChar to char16_t. For non-ICU codes, UChar can be configured to what used to be and it works fine with just one change in ToT blink. However, it leads to a goma failure. See https://codereview.chromium.org/2738453005/ (v8) and https://codereview.chromium.org/2740673002/ (chromium/blink ). So, I tried to make UChar always char16_t within and outside ICU and multiple definition of ctor errors and found this change.
Message was sent while issue was closed.
How is this different from https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/editing/c... ? We've had that for years I think. Are UChar and char16_t different sizes? |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
