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.makela@mariadb.com wrote:
commit 96403c5e1240342948cb88600ece14c0c8b72dbc Author: Marko Mäkelä <marko.makela@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)
{ int2store(Ptr + str_length, n); str_length += 2; } - void q_append(const uint32 n) + void q_append(size_t n) {
psergey: this just appends the two lower bytes. Is this change really needed? 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