Hi!

I would like to add a few more arguments in favour of using const references and not const pointers. I do not endorse non-const references, that makes the code silly and confusing a lot of the times. When we need output or input-output parameters we should keep using pointers. For strictly mandatory (must never be null) input parameters, const references are the way to go, unless there is a strong reason not to.

The guarantee that references enforce is "never null". This is a very important and useful feature of C++. We can completely skip any checks such as:

func1(LEX_CSTRING *str)
{
   if (!str) { .. }
}

Or DBUG_ASSERTS such as:
func1(LEX_CSTRING *str)
{
    DBUG_ASSERT(str);
    ...
}

The code with *const references*  is self documenting in this sense. It makes it very clear that the function does not own the object it is being passed (no questions wether the pointer should be freed or not) and that the reference is valid.

Other arguments, not strictly related to this use case, but are in favor of const references:

* By limiting our use of references to const references we avoid the ambiguity between regular pass-by-value arguments and non-const reference arguments. You work with them as if they are pass-by-value. Since they are const you can't do anything to them anyway, you're not changing any of the caller's data.
* Pointers are unclear wether they point to a buffer array or a single item. References do not allow you to do: ptr[0], ptr[1], ptr[2], etc.
* If we chose to make use of STL algorithms or C++11 features down the line, references tend to work easier than pointers. Can't come up with a good example right this moment, but most stl algorithms expect reference-based parameters, ex:
std::max(*int_ptr_a, *int_ptr_b) vs std::max(int_a, int_b)
* If some legacy code does need a pointer parameter, using the unary & operator on the reference will produce the required pointer type.

If we are doing refactoring anyway, I strongly request we reconsider the policy of avoiding references completely. I suggest we at least make use of them as input parameters to functions and methods in new code.

Vicențiu



On Thu, 20 Apr 2017 at 00:10 Alexander Barkov <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);
}




I'd propose two things instead:

1. Add a class:


class Lex_cstring: public LEX_CSTRING
{
public:
  Lex_cstring() {} /*Uninitialized */
  Lex_cstring(const char *str, size_t length)
  {
    LEX_STRING::str= str;
    LEX_STRING::length= length;
  }
  Lex_cstring(const LEX_STRING *str)
  {
    LEX_STRING::str= str->str;
    LEX_STRING::length= str->length;
  }
};


2. Instead of "const LEX_CSTRING *str"
we use "const Lex_cstring &str" all around C++ code.

So the above example with passing a LEX_STRING to
something that expect a LEX_CSTRING would be simplified to

void func1(const LEX_STRING *str)
{
  func2(str);
}


3. In some classes now we have duplicate methods and constructors like this:

class XXX
{
public:
  XXX(const char *str, size_t length) { ... }
  XXX(const LEX_STRING *str) { ... }
  void method(const char *str, size_t length) {...}
  void method(const LEX_STRING *str) {...}
};

to make it possible to call like this:

  new XXX(str, length);
  new XXX(lex_str_ptr)
  new XXX(&lex_str)
  method(str, length);
  method(lex_str_ptr);
  method(&lex_str);


We change this to "passing by reference" and remove duplicates:

class XXX
{
public:
  XXX(const Lex_cstring &str) { ... }
  void method(const Lex_cstring &str) {...}
};

After this change we can call like this:

  new XXX(Lex_cstring(str, length));
  new XXX(*lex_str_ptr)
  new XXX(lex_str);
  method(Lex_cstring(str, length));
  method(*lex_str_ptr);
  method(lex_str);

Notice, not needs for separate methods accepting
two arguments "const char *str, size_t length".