New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 652586 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Sep 14
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

WTF::String should use one of unsigned or size_t instead of mixed

Project Member Reported by yosin@chromium.org, Oct 4 2016

Issue description

In WTF::String, character index and length are in |unsigned| but |find()| and family return |size_t|.

To avoid 64-bit to 32-bit truncation warning and mixing of |unsigned| and |size_t| in callers, we should use one of them in |WTF::String|.
 
Status: Available (was: Untriaged)
Yeah we should probably make this consistent.

Comment 2 by drott@chromium.org, Oct 24 2016

I'd vote for size_t, this was flagged by the compiler as well when I was compiling with -Wconversion as well.

Project Member

Comment 3 by sheriffbot@chromium.org, Oct 24 2017

Labels: Hotlist-Recharge-Cold
Status: Untriaged (was: Available)
This issue has been Available for over a year. If it's no longer important or seems unlikely to be fixed, please consider closing it out. If it is important, please re-triage the issue.

Sorry for the inconvenience if the bug really should have been left as Available. If you change it back, also remove the "Hotlist-Recharge-Cold" label.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Project Member

Comment 4 by bugdroid1@chromium.org, Jul 27

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

commit 9d0fd86cf833cd7f35eea342f15c827516938d67
Author: Dave Tapuska <dtapuska@chromium.org>
Date: Fri Jul 27 17:14:32 2018

Introduce wtf_size_t which is typedef for uint32_t.

WTF vector and strings cannot be larger than UINT_MAX so change the
indexing, length and find methods to return wtf_size_t.

The desire to change this to the maximum size and not align it with
std::vector which returns size_t is such that callers actually know
the desired maximum size. One example is PaintChunks are currently using
size_t indicies but the size of PaintChunks can be reduced by 8 bytes
(moving two indicies to be 32 bit from 64 bit on 64 bit platforms).

This change is a minimal change. Progressively all the call sites will be
fixed such that they are using wtf_size_t.
https://chromium-review.googlesource.com/c/chromium/src/+/1128178
is a prototype of all the fixes but is to large to commit by itself since
approximately 1100 files need to be fixed.

BUG= 652586 

Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;luci.chromium.try:linux_layout_tests_layout_ng;luci.chromium.try:linux_layout_tests_slimming_paint_v2;luci.chromium.try:linux_optional_gpu_tests_rel;luci.chromium.try:mac_optional_gpu_tests_rel;luci.chromium.try:win_optional_gpu_tests_rel;master.tryserver.blink:linux_trusty_blink_rel
Change-Id: I77db5df1313e6ab92497d4a811204707d5419c53
Reviewed-on: https://chromium-review.googlesource.com/1145045
Commit-Queue: Dave Tapuska <dtapuska@chromium.org>
Reviewed-by: Yuta Kitamura <yutak@chromium.org>
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Cr-Commit-Position: refs/heads/master@{#578684}
[modify] https://crrev.com/9d0fd86cf833cd7f35eea342f15c827516938d67/third_party/blink/public/platform/DEPS
[modify] https://crrev.com/9d0fd86cf833cd7f35eea342f15c827516938d67/third_party/blink/public/platform/web_vector.h
[modify] https://crrev.com/9d0fd86cf833cd7f35eea342f15c827516938d67/third_party/blink/renderer/core/css/css_value.h
[modify] https://crrev.com/9d0fd86cf833cd7f35eea342f15c827516938d67/third_party/blink/renderer/core/css/parser/css_parser_token_range.h
[modify] https://crrev.com/9d0fd86cf833cd7f35eea342f15c827516938d67/third_party/blink/renderer/core/fetch/multipart_parser.cc
[modify] https://crrev.com/9d0fd86cf833cd7f35eea342f15c827516938d67/third_party/blink/renderer/core/html/forms/date_time_edit_element.cc
[modify] https://crrev.com/9d0fd86cf833cd7f35eea342f15c827516938d67/third_party/blink/renderer/core/html/forms/html_select_element.cc
[modify] https://crrev.com/9d0fd86cf833cd7f35eea342f15c827516938d67/third_party/blink/renderer/core/html/parser/html_parser_idioms.cc
[modify] https://crrev.com/9d0fd86cf833cd7f35eea342f15c827516938d67/third_party/blink/renderer/core/html/parser/html_parser_idioms.h
[modify] https://crrev.com/9d0fd86cf833cd7f35eea342f15c827516938d67/third_party/blink/renderer/core/html/parser/xss_auditor.cc
[modify] https://crrev.com/9d0fd86cf833cd7f35eea342f15c827516938d67/third_party/blink/renderer/core/layout/ng/ng_container_fragment_builder.cc
[modify] https://crrev.com/9d0fd86cf833cd7f35eea342f15c827516938d67/third_party/blink/renderer/core/svg/properties/svg_list_property_helper.h
[modify] https://crrev.com/9d0fd86cf833cd7f35eea342f15c827516938d67/third_party/blink/renderer/core/svg/svg_string_list.cc
[modify] https://crrev.com/9d0fd86cf833cd7f35eea342f15c827516938d67/third_party/blink/renderer/modules/webaudio/audio_param_timeline.cc
[modify] https://crrev.com/9d0fd86cf833cd7f35eea342f15c827516938d67/third_party/blink/renderer/modules/webgl/webgl_rendering_context_base.cc
[modify] https://crrev.com/9d0fd86cf833cd7f35eea342f15c827516938d67/third_party/blink/renderer/platform/bindings/to_v8.h
[modify] https://crrev.com/9d0fd86cf833cd7f35eea342f15c827516938d67/third_party/blink/renderer/platform/graphics/paint/paint_chunk.cc
[modify] https://crrev.com/9d0fd86cf833cd7f35eea342f15c827516938d67/third_party/blink/renderer/platform/heap/gc_info.h
[modify] https://crrev.com/9d0fd86cf833cd7f35eea342f15c827516938d67/third_party/blink/renderer/platform/heap/heap_allocator.h
[modify] https://crrev.com/9d0fd86cf833cd7f35eea342f15c827516938d67/third_party/blink/renderer/platform/heap/heap_test.cc
[modify] https://crrev.com/9d0fd86cf833cd7f35eea342f15c827516938d67/third_party/blink/renderer/platform/heap/persistent.h
[modify] https://crrev.com/9d0fd86cf833cd7f35eea342f15c827516938d67/third_party/blink/renderer/platform/heap/threading_traits.h
[modify] https://crrev.com/9d0fd86cf833cd7f35eea342f15c827516938d67/third_party/blink/renderer/platform/heap/trace_traits.h
[modify] https://crrev.com/9d0fd86cf833cd7f35eea342f15c827516938d67/third_party/blink/renderer/platform/shared_buffer.h
[modify] https://crrev.com/9d0fd86cf833cd7f35eea342f15c827516938d67/third_party/blink/renderer/platform/web_icon_sizes_parser.cc
[modify] https://crrev.com/9d0fd86cf833cd7f35eea342f15c827516938d67/third_party/blink/renderer/platform/wtf/deque.h
[modify] https://crrev.com/9d0fd86cf833cd7f35eea342f15c827516938d67/third_party/blink/renderer/platform/wtf/deque_test.cc
[modify] https://crrev.com/9d0fd86cf833cd7f35eea342f15c827516938d67/third_party/blink/renderer/platform/wtf/doubly_linked_list.h
[modify] https://crrev.com/9d0fd86cf833cd7f35eea342f15c827516938d67/third_party/blink/renderer/platform/wtf/forward.h
[modify] https://crrev.com/9d0fd86cf833cd7f35eea342f15c827516938d67/third_party/blink/renderer/platform/wtf/hash_map.h
[modify] https://crrev.com/9d0fd86cf833cd7f35eea342f15c827516938d67/third_party/blink/renderer/platform/wtf/hash_set.h
[modify] https://crrev.com/9d0fd86cf833cd7f35eea342f15c827516938d67/third_party/blink/renderer/platform/wtf/hash_set_test.cc
[modify] https://crrev.com/9d0fd86cf833cd7f35eea342f15c827516938d67/third_party/blink/renderer/platform/wtf/not_found.h
[modify] https://crrev.com/9d0fd86cf833cd7f35eea342f15c827516938d67/third_party/blink/renderer/platform/wtf/ref_vector.h
[modify] https://crrev.com/9d0fd86cf833cd7f35eea342f15c827516938d67/third_party/blink/renderer/platform/wtf/text/atomic_string.h
[modify] https://crrev.com/9d0fd86cf833cd7f35eea342f15c827516938d67/third_party/blink/renderer/platform/wtf/text/cstring.cc
[modify] https://crrev.com/9d0fd86cf833cd7f35eea342f15c827516938d67/third_party/blink/renderer/platform/wtf/text/cstring.h
[modify] https://crrev.com/9d0fd86cf833cd7f35eea342f15c827516938d67/third_party/blink/renderer/platform/wtf/text/string_builder.h
[modify] https://crrev.com/9d0fd86cf833cd7f35eea342f15c827516938d67/third_party/blink/renderer/platform/wtf/text/string_concatenate.cc
[modify] https://crrev.com/9d0fd86cf833cd7f35eea342f15c827516938d67/third_party/blink/renderer/platform/wtf/text/string_impl.cc
[modify] https://crrev.com/9d0fd86cf833cd7f35eea342f15c827516938d67/third_party/blink/renderer/platform/wtf/text/string_impl.h
[modify] https://crrev.com/9d0fd86cf833cd7f35eea342f15c827516938d67/third_party/blink/renderer/platform/wtf/text/string_utf8_adaptor.h
[modify] https://crrev.com/9d0fd86cf833cd7f35eea342f15c827516938d67/third_party/blink/renderer/platform/wtf/text/wtf_string.cc
[modify] https://crrev.com/9d0fd86cf833cd7f35eea342f15c827516938d67/third_party/blink/renderer/platform/wtf/text/wtf_string.h
[modify] https://crrev.com/9d0fd86cf833cd7f35eea342f15c827516938d67/third_party/blink/renderer/platform/wtf/vector.h
[modify] https://crrev.com/9d0fd86cf833cd7f35eea342f15c827516938d67/third_party/blink/renderer/platform/wtf/vector_test.cc
[add] https://crrev.com/9d0fd86cf833cd7f35eea342f15c827516938d67/third_party/blink/renderer/platform/wtf/wtf_size_t.h

Cc: jose.dap...@lge.com
Owner: dtapu...@chromium.org
Status: Started (was: Untriaged)
Dave, do you have plans to land the mojo/public/cpp/bindings/array_traits_wtf_vector.h part of https://chromium-review.googlesource.com/c/chromium/src/+/1128178 soon?

The CL above has broken the GCC build because the templates don't match:

In file included from ../../mojo/public/cpp/bindings/array_traits_span.h:11,
                 from ../../mojo/public/cpp/bindings/lib/serialization.h:13,
                 from gen/net/interfaces/host_resolver_service.mojom-blink.h:25,
                 from gen/net/interfaces/host_resolver_service.mojom-blink.cc:15:
../../mojo/public/cpp/bindings/array_traits.h: In instantiation of ‘struct mojo::ArrayTraits<WTF::Vector<mojo::StructPtr<net::interfaces::blink::IPEndPoint> > >’:
../../mojo/public/cpp/bindings/lib/template_util.h:74:30:   required from ‘struct mojo::internal::EnsureTypeIsComplete<mojo::ArrayTraits<WTF::Vector<mojo::StructPtr<net::interfaces::blink::IPEndPoint> > > >’
../../mojo/public/cpp/bindings/lib/serialization_util.h:146:27:   required from ‘struct mojo::internal::HasGetBeginMethod<mojo::ArrayTraits<WTF::Vector<mojo::StructPtr<net::interfaces::blink::IPEndPoint> > >, const WTF::Vector<mojo::StructPtr<net::interfaces::blink::IPEndPoint> > >’
../../mojo/public/cpp/bindings/lib/array_serialization.h:470:74:   required from ‘struct mojo::internal::Serializer<mojo::ArrayDataView<net::interfaces::IPEndPointDataView>, const WTF::Vector<mojo::StructPtr<net::interfaces::blink::IPEndPoint> > >’
../../mojo/public/cpp/bindings/lib/serialization_forward.h:43:16:   required from ‘void mojo::internal::Serialize(InputUserType&&, Args&& ...) [with MojomType = mojo::ArrayDataView<net::interfaces::IPEndPointDataView>; InputUserType = const WTF::Vector<mojo::StructPtr<net::interfaces::blink::IPEndPoint> >&; Args = {mojo::internal::Buffer*&, mojo::internal::Array_Data<mojo::internal::Pointer<net::interfaces::internal::IPEndPoint_Data> >::BufferWriter*, const mojo::internal::ContainerValidateParams*, mojo::internal::SerializationContext*&}; typename std::enable_if<(! mojo::internal::IsOptionalWrapper<InputUserType>::value)>::type* <anonymous> = 0]’
gen/net/interfaces/address_list.mojom-shared.h:115:90:   required from ‘static void mojo::internal::Serializer<net::interfaces::AddressListDataView, MaybeConstUserType>::Serialize(MaybeConstUserType&, mojo::internal::Buffer*, net::interfaces::internal::AddressList_Data::BufferWriter*, mojo::internal::SerializationContext*) [with MaybeConstUserType = mojo::StructPtr<net::interfaces::blink::AddressList>]’
../../mojo/public/cpp/bindings/lib/serialization_forward.h:43:16:   required from ‘void mojo::internal::Serialize(InputUserType&&, Args&& ...) [with MojomType = net::interfaces::AddressListDataView; InputUserType = mojo::StructPtr<net::interfaces::blink::AddressList>&; Args = {mojo::internal::Buffer*&, net::interfaces::internal::AddressList_Data::BufferWriter*, mojo::internal::SerializationContext*}; typename std::enable_if<(! mojo::internal::IsOptionalWrapper<InputUserType>::value)>::type* <anonymous> = 0]’
gen/net/interfaces/host_resolver_service.mojom-blink.cc:96:64:   required from here
../../mojo/public/cpp/bindings/array_traits.h:72:17: error: static assertion failed: Cannot find the mojo::ArrayTraits specialization. Did you forget to include the corresponding header file?
   static_assert(internal::AlwaysFalse<T>::value,
                 ^~~~~~~~

Can you let me know if https://chromium-review.googlesource.com/c/chromium/src/+/1154990 fixes it for you?
It does, thank you!
Good, sorry about that. 
Project Member

Comment 9 by bugdroid1@chromium.org, Jul 30

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

commit ab027ae0b4f60b704322f4cb291270412eca1390
Author: Dave Tapuska <dtapuska@chromium.org>
Date: Mon Jul 30 15:15:11 2018

Fix GCC build issue with WTF Vector change

Follow up from https://chromium.googlesource.com/chromium/src.git/+/9d0fd86cf833cd7f35eea342f15c827516938d67

Adjust mojo template. It built correctly with clang but failed to build
with GCC.

BUG= 652586 , 819294
TBR=jam@chromium.org

Change-Id: I985bf855d5fce1f2c774123a8f6103e97aedb9f6
Reviewed-on: https://chromium-review.googlesource.com/1154990
Commit-Queue: Dave Tapuska <dtapuska@chromium.org>
Reviewed-by: Raphael Kubo da Costa (CET) <raphael.kubo.da.costa@intel.com>
Cr-Commit-Position: refs/heads/master@{#579040}
[modify] https://crrev.com/ab027ae0b4f60b704322f4cb291270412eca1390/mojo/public/cpp/bindings/array_traits_wtf_vector.h

Status: Fixed (was: Started)

Sign in to add a comment