List:Commits« Previous MessageNext Message »
From:Alexander Barkov Date:December 6 2010 12:50pm
Subject:Re: bzr commit into mysql-5.1-bugteam branch (mats.kindahl:3508)
Bug#58246
View as plain text  
Hi,

Georgi Kodinov wrote:
> Hi Mats,
> 
> Here's my take on the fix (http://lists.mysql.com/commits/124827) : 
> 
> I think my_strspn/my_strcspn are not fully character set aware, they assume that: 
> a) the delimiters string is in a 1-byte character set
> b) the string to compare it with is also in 1-byte character set
> 
> While this is probably ok by a stretch with most of the OSes (since the delimiters in
> UTF-8 are 1 byte and I don't think there're valid letters starting with the same byte as
> the delimiters) it's not correct in the general case.
> 
> I see that my_strchr() suffers the same limitations.
> IMHO we either implement a proper character set agnostic
> my_strchr/my_strspn/my_strcspn or we don't put it in the standard library. 
> 
> Moreover we have a TODO: in my_strchr.c: 
> "TODO: should be moved to CHARSET_INFO if it's going to be called frequently."
> 
> All that said I think that it's probably OK to add these two functions there as long
> as the limitations are clearly explained in a comment and a TODO is added to move them to
> CHARSET_INFO similarly to my_strchr(). But I will leave this to Bar to decide (I've cc-ed
> him).

I think it's better not to add a new function,
because we already have a thing like this:
a function in MY_CHARSET_HANDLER:

size_t (*scan)(struct charset_info_st *,
           const char *b, const char *e, int sq);

It returns the length of the initial segment of the string
which consist entirely of characters of a certain sequence "sq".

It currently understands these types in "sq":

m_ctype.h:#define MY_SEQ_INTTAIL  1
m_ctype.h:#define MY_SEQ_SPACES   2

I suggest to extend this function with a new sequence type,
say MY_SEQ_NODIR_FILENAME.

Note, you don't need to implement this in ctype-ucs2.c,
a DEBUG_ASSERT(0) is enough.


> 
> One additional question is why are you adding a function that nobody is using
> (my_strspn) ? 
> This is a sure recipe for problems later and contributes to the coverage problem that
> we have.
> If you want to explain how it's done add a comment, not a dead function. 
> 
> Other than this I don't have objections to the path other than some trivia like e.g.
> adding an inline to the check_if_path() definition (and probably calling it somewhat
> differently, e.g. has_no_path_delimiters()).
> 
> Best Regards,
> Joro

Thread
bzr commit into mysql-5.1-bugteam branch (mats.kindahl:3508) Bug#58246Mats Kindahl1 Dec
Re: bzr commit into mysql-5.1-bugteam branch (mats.kindahl:3508)Bug#58246Alexander Barkov6 Dec