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
Sergey Petrunia