Hi Monty, On 04/20/2017 09:35 AM, Michael Widenius wrote:
Hi!
On 20 Apr 2017 06:10, "Alexander Barkov" <bar@mariadb.org <mailto:bar@mariadb.org>> wrote:
Hello all,
Monty is now doing refactoring:
1. Replacing a lot of LEX_STRING to LEX_CSTRING.
2. Fixing functions and methods that have arguments of types: a. pointer "const LEX_STRING *name" and b. reference "const LEX_STRING &name". to the same style. Monty chose passing by pointer (2a).
I think that:
- #1 is good. - #2 is good, in the sense of changing to the same style.
But I think that the choice of "passing by pointer" in #2 is bad. Passing by reference for LEX_STRING/LEX_CSTRING related things would be better.
With the "passing by pointer" approach whenever one needs to pass a LEX_STRING to a function accepting LEX_CSTRING, one will have to do:
void func1(const LEX_STRING *str) { LEX_CSTRING tmp= {str->str, str->length}; func2(&tmp); }
The above is not a problem:
- We don't have less than 5 places in the code where LEX_STRING is changed to LEX_CSTRING. - In these places one can do this with a simple cast, nothing elaborated.
There are more examples: Item_str_func(THD *thd): Item_func(thd) { decimals=NOT_FIXED_DEC; } Item_str_func(THD *thd, Item *a): Item_func(thd, a) {decimals=NOT_FIXED_DEC; } Item_str_func(THD *thd, Item *a, Item *b): Item_func(thd, a, b) { decimals=NOT_FIXED_DEC; } Item_str_func(THD *thd, Item *a, Item *b, Item *c): Item_func(thd, a, b, c) { decimals=NOT_FIXED_DEC; } Item_str_func(THD *thd, Item *a, Item *b, Item *c, Item *d): Item_func(thd, a, b, c, d) { decimals=NOT_FIXED_DEC; } Item_str_func(THD *thd, Item *a, Item *b, Item *c, Item *d, Item* e): Item_func(thd, a, b, c, d, e) { decimals=NOT_FIXED_DEC; } Item_str_func(THD *thd, List<Item> &list): Item_func(thd, list) { decimals=NOT_FIXED_DEC; } This is terrible. The same thing in Item_int_func, Item_real_func, Item_decimal_func, etc, etc. There is only one constructor needed for all of them. For example, for Item_str_func: Item_str_func(THD *thd, const Item_args &args): Item_func(thd, args) { decimals=NOT_FIXED_DEC; } And when you need to call new, you do: Item *item= new (thd->mem_root) Item_func_somefunc(thd, Item_args(a,b,c)); You cannot do this with the "pass by pointer" approach.
I'd propose two things instead:
1. Add a class:
No reason to make a complex solution for a problem that doesn't exist.
It's a trivial solution. The problem does exists. The current code absolutely is excessive and inconvenient to use.
2. Instead of "const LEX_CSTRING *str" we use "const Lex_cstring &str" all around C++ code.
No, no, no.
I do not want to use & anywhere as arguments in the code. It hides things and lends people to do errors like we had before where they declared some functions to take full structure she references in other places, which creates a mess.
I don't propose to use pass-by-value: func(Some_class arg); I propose to use pass-by-reference: func(const Some_class &) When you do: func(some_class_instance); pass-by-value requires a constructor call, indeed. but pass-by-reference does not need a constructor call! pass-by-reference compiles to exactly the same thing with pass-by-pointer: it puts the address of the object to the stack. Nothing else.
Regards, Monty