List:Internals« Previous MessageNext Message »
From:Michael Widenius Date:October 14 2009 4:13pm
Subject:re: String:c_ptr makes me unhappy
View as plain text  

>>>>> "MARK" == MARK CALLAGHAN <mdcallag@stripped> writes:

MARK> Have you wasted time on valgrind warnings from code that uses
MARK> String:c_ptr instead of String:c_ptr_safe?
MARK> Have you fixed bugs caused by the use of String:c_ptr instead of
MARK> String::c_ptr_safe?

MARK> I have and I am not happy about it. Over on another mailing list I
MARK> read about another bug caused by using c_ptr instead of c_ptr_safe. It
MARK> wasn't Drizzle, as they appear to have made c_ptr do the right thing.

MARK> There are a few changes that can improve this:

MARK> 1) Add comments to sql_string.h. There are none today in 5.0 and 5.1
MARK> in sql_string.h for c_ptr() and c_ptr_unsafe(). I know that using
MARK> comments in header files rather than source files has been
MARK> controversial in MySQL. AFAIK, that issue is limited to MySQL or ctags
MARK> users within MySQL devel. I have never encountered a project where
MARK> that wasn't done or at least considered the right thing to do.

The main reason for me personally having comments in the code instead
of the header file, is that in the .c and .cc files you can have much
more description about the function and in practice I never use the .h
files to look for things.

However, other peoples way to work differ and I can see a good reason
to provide some documentation in the .h file too.

MARK> 2) Change the name of c_ptr as it doesn't return a C pointer to a
MARK> string. If it did, the result would be nul terminated and there would
MARK> be no need for c_ptr_unsafe.

c_ptr does return a pointer to a \0 terminated string. It does however
don't know if the last byte is initialized memory or not, which is a
problem for valgrind but not normally for MySQL as Strings is mainly
used for thread specific memory.

There is no c_ptr_unsafe, only c_ptr_safe. The reason to have both
version is the few cases where we know that the String is not in
thread specific memory and it's worth the extra malloc + copy to
access the string.

In most cases the proper solution is of course to get rid of all
c-terminated strings, in which case we don't have a problem anymore...

String:c_ptr makes me unhappyMARK CALLAGHAN14 Oct
  • re: String:c_ptr makes me unhappyMichael Widenius14 Oct
    • Re: String:c_ptr makes me unhappyMARK CALLAGHAN14 Oct
      • Re: String:c_ptr makes me unhappyMichael Widenius14 Oct
        • Re: String:c_ptr makes me unhappyKristian Nielsen15 Oct
          • Re: String:c_ptr makes me unhappyMichael Widenius15 Oct