On Wed, 6 Jan 2016 at 21:48 Sergei Golubchik <serg@mariadb.org> wrote:
Hi, Vicențiu!

Hi Sergei!
 
On Jan 05, Vicențiu Ciorbaru wrote:
>
> Sample code where we can avoid ifs:
>
>     case SSL_TYPE_SPECIFIED:
>       table->field[next_field]->store(STRING_WITH_LEN("SPECIFIED"),
>                                       &my_charset_latin1);
>       table->field[next_field+1]->store("", 0, &my_charset_latin1);
>       table->field[next_field+2]->store("", 0, &my_charset_latin1);
>       table->field[next_field+3]->store("", 0, &my_charset_latin1);
>       if (lex->ssl_cipher)
>         table->field[next_field+1]->store(lex->ssl_cipher,
>                                           strlen(lex->ssl_cipher),
>                                           system_charset_info);
>       if (lex->x509_issuer)
>         table->field[next_field+2]->store(lex->x509_issuer,
>                                           strlen(lex->x509_issuer),
>                                           system_charset_info);
>       if (lex->x509_subject)
>         table->field[next_field+3]->store(lex->x509_subject,
>                                           strlen(lex->x509_subject),
>                                           system_charset_info);
>
> This just becomes:
>     case SSL_TYPE_SPECIFIED:
>       table->field[next_field]->store(STRING_WITH_LEN("SPECIFIED"), &my_charset_latin1);
>       table->field[next_field+1]->store(ssl_cipher.c_str(), ssl_cipher.size(), &my_charset_latin1);
>       table->field[next_field+2]->store(x509_issuer.c_str(), x509_issuer.size(), &my_charset_latin1);
>       table->field[next_field+3]->store(x509_subject.c_str(), x509_subject.size(), &my_charset_latin1);
>
> Thoughts?

I can immediately think of two more approaches:

 1. using String class (sql/sql_string.h)
 2. using safe_str helper:

   table->field[next_field+1]->store(safe_str(lex->ssl_cipher),
                                     safe_strlen(lex->ssl_cipher),
                                     &my_charset_latin1);


As for std:string - currently the server code uses these approaches to
storing strings: char*, LEX_STRING, DYNAMIC_STRING, CSET_STRING, String.
With variations like uchar*, LEX_CSTRING, LEX_CUSTRING, StringBuffer,
may be more.

I don't particularly like an idea of adding yes another alternative into
this mess without a clear rule of what to use where.
That's what I feel would be an issue too.
 
So, could you say when should one use std::string and when one should
stick to one of the existing types?

The place where using the std::string would be preferable, I'd say is where we don't make use of a memroot to automatically take care of memory allocations. This is indeed rare however.

Another interesting alternative is when we copy the string a few times. We talked about the move semantics in Germany. Using a std::string might allow the compiler to optimise such copies as it has optimisations specifically tailored for std::string.

Another problem with LEX_STRING and it's derivatives is that we do not actually enforce const-ness during compile time.

f(const LEX_STRING* s) {
    s->str[1] = 'b';
}

This function compiles perfectly but the const parameter does not help in detecting this bug. I agree that this kind of code clearly "smells", but it is another thing we need to watch out for.

When considering ownership of a string, it is sometimes unclear if we own the string or not and you need to check the caller to understand where the string parameter actually comes from.

For the use-case I mentioned, we can do just fine without std::string, but I do think that std::string could potentially replace most of our in-house strings, with the added benefit of handling memory allocations, where memroot might not be an option and having the extra type safety that comes with using it.

Regards,
Vicentiu