Hi Marko, Please find below the review for patch #1 of the "size_t patches": my comments are marked with 'psergey:'
On Thu, Mar 30, 2017 at 02:39:50PM +0300, marko.mak...@mariadb.com wrote: > commit 96403c5e1240342948cb88600ece14c0c8b72dbc > Author: Marko Mäkelä <marko.mak...@mariadb.com> > Date: Sat Mar 11 22:03:31 2017 +0200 > > Replace some unsigned integers with size_t to avoid type mismatch. > > diff --git a/sql/key.cc b/sql/key.cc > index bb10e90..9e8693f 100644 > --- a/sql/key.cc > +++ b/sql/key.cc > @@ -1,4 +1,5 @@ > /* Copyright (c) 2000, 2010, Oracle and/or its affiliates. All rights > reserved. > + Copyright (c) 2017, MariaDB Corporation. > > This program is free software; you can redistribute it and/or modify > it under the terms of the GNU General Public License as published by > @@ -392,7 +393,7 @@ void field_unpack(String *to, Field *field, const uchar > *rec, uint max_length, > tmp.length(charpos); > } > if (max_length < field->pack_length()) > - tmp.length(min(tmp.length(),max_length)); > + tmp.length(min(tmp.length(),static_cast<size_t>(max_length))); psergey: This is not necessary as tmp.length() returns a 'size_t', but it's still ok to have I guess. > ErrConvString err(&tmp); > to->append(err.ptr()); > } ... > diff --git a/sql/spatial.cc b/sql/spatial.cc > index 7c9d8bb..399a968 100644 > --- a/sql/spatial.cc > +++ b/sql/spatial.cc > @@ -2278,7 +2278,7 @@ int Gis_multi_line_string::geometry_n(uint32 num, > String *result) const > break; > data+= length; > } > - return result->append(data, length, (uint32) 0); > + return result->append(data, length, static_cast<size_t>(0)); psergey: isn't that excessive, can one just write "0" there? > } > > > @@ -2710,8 +2710,9 @@ int Gis_multi_polygon::geometry_n(uint32 num, String > *result) const > } while (--num); > if (no_data(data, 0)) // We must check last > segment > return 1; > - return result->append(start_of_polygon, (uint32) (data - start_of_polygon), > - (uint32) 0); > + return result->append(start_of_polygon, > + static_cast<size_t>(data - start_of_polygon), > + static_cast<size_t>(0)); psergey: isn't that excessive, can one just write "0" there? > } > > ... > diff --git a/sql/sql_string.h b/sql/sql_string.h > index 18f5f4c..8248f6d 100644 > --- a/sql/sql_string.h > +++ b/sql/sql_string.h > @@ -531,16 +531,19 @@ class String > { > Ptr[str_length++] = c; > } > - void q_append2b(const uint32 n) > + void q_append2b(size_t n) psergey: this just appends the two lower bytes. Is this change really needed? > { > int2store(Ptr + str_length, n); > str_length += 2; > } > - void q_append(const uint32 n) > + void q_append(size_t n) > { psergey: This always appends 4 bytes. It is a little bit scary that this function will optionally discard the upper 4 bytes of the passed values. As far as I understand its usage, this is for writing the data that should always be 4 bytes, regardless of the sizeof(size_t) of the platform we're on. > int4store(Ptr + str_length, n); > str_length += 4; > } > +#if SIZEOF_SIZE_T > 4 > + void q_append(uint32 n) { q_append(static_cast<size_t>(n)); } > +#endif > void q_append(double d) > { > float8store(Ptr + str_length, d); ... > @@ -669,11 +672,11 @@ class String > { > return !sortcmp(this, other, cs); > } > - void q_net_store_length(ulonglong length) > + void q_net_store_length(size_t length) psergey: I dont think this should vary depending on sizeof(size_t). This function is used to construct data to be sent over network, check sql/protocol.cc, net_send_ok(). Apparently network data should have the same number of bits always. > { > DBUG_ASSERT(Alloced_length >= (str_length + net_length_size(length))); > char *pos= (char *) net_store_length((uchar *)(Ptr + str_length), > length); > - str_length= uint32(pos - Ptr); > + str_length= static_cast<size_t>(pos - Ptr); > } > void q_net_store_data(const uchar *from, size_t length) > { BR Sergei -- Sergei Petrunia, Software Developer MariaDB Corporation | Skype: sergefp | Blog: http://s.petrunia.net/blog _______________________________________________ Mailing list: https://launchpad.net/~maria-developers Post to : maria-developers@lists.launchpad.net Unsubscribe : https://launchpad.net/~maria-developers More help : https://help.launchpad.net/ListHelp