[Maria-developers] Use of std::string
Hi Sergei, I was doing some work within sql_acl.cc, having to refactor a bit of logic regarding USER_RESOURCES and SSL related stuff and I'm thinking of making use of std::string instead of const char* to remove the need for checking NULL pointers. I noticed that we generally do not make any use of std::string within the server code, apart from storage engines. The use cases are not performance sensitive so any overhead caused by std::string I think is smaller than the benefit of not having if (!NULL) statements everywhere. I know that it is obviously based on the final result, but if it is a definite no from the start, I'll implement things using regular C pointers. Also, this is only for the new things I'm implementing, I'm not planning on refactoring everything. 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? Regards, Vicentiu
Hi, Vicențiu! 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. So, could you say when should one use std::string and when one should stick to one of the existing types? Regards, Sergei Chief Architect MariaDB and security@mariadb.org -- Vote for my Percona Live 2016 talks: https://www.percona.com/live/data-performance-conference-2016/sessions/maria... https://www.percona.com/live/data-performance-conference-2016/sessions/maria...
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
participants (2)
-
Sergei Golubchik -
Vicențiu Ciorbaru