maria-developers team mailing list archive

[Sergey Petrunia <sergey@xxxxxxxxxxx>]

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@xxxxxxxxxxx wrote:
> commit 96403c5e1240342948cb88600ece14c0c8b72dbc
> Author: Marko Mäkelä <marko.makela@xxxxxxxxxxx>
> 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