Re: [Maria-developers] Coding Style Review INSERT RETURNING
Hello!
Thanks for the review. I'll start working on it and keep you updated with
my progress.
Regards,
Rucha
On Wed, Aug 7, 2019 at 7:51 PM Vicențiu Ciorbaru
wrote:
> Hi Rucha!
>
> This is going to be a very thorough review looking at coding style. I'll
> nitpick on every line of code where I find something, just be patient and
> let's clean up everything.
>
> You might consider me picking on the coding style a bit pedantic, but it
> really is important. It is important because it helps in searching, it
> helps
> in understanding the code faster, it helps to look-up history faster.
> The good part is that once you get used to writing things properly, you
> won't
> even have to think about it actively. It will just come naturally.
>
> I suspect you do not have your editor configured to show whitespaces and
> show you tabs versus spaces. Please look into your editor's settings to
> highlight both tabs and spaces.
>
> There are scripts that remove trailing whitespaces for you, across whole
> files.
> I don't recommend them for MariaDB code as we have remnants from MySQL
> times of
> these and because we care more about preserving the immediate history for
> running `git blame`, we did not fully fix all files. For any other
> project, I
> highly recommend you use them.
>
> Some of these comments might slightly contradict what I said before. That
> is
> because I wanted us to get things working first and I didn't want to burden
> you with too many details initially. I'll look to explain myself now
> clearly
> so you understand why some things are bad ideas. I want you to take this
> review also as a general guideline for any other code you write, anywhere.
> With these hints, tricks, ideas, you'll produce much higher quality code in
> the long run. It will take you longer to write sometimes, but it really
> makes
> a difference. :)
>
> Also, you may have noticed some parts of our code do not agree with my
> general
> guidelines. You have to take into account that this code was written over
> the
> course of many years, by different people, with different levels of
> experience
> or available time to implement something. Try not to make use of it as an
> excuse for new code that you write. It's up to you to always write the best
> possible version *with the time allowed*. We have adequate time now so we
> can afford to look for better solutions.
>
> With all that said, this took me a while to write and I may still
> have missed a few things. Feel free to point out anything that looks
> strange.
> Now, let's get to the review.
>
> >
> > diff --git a/sql/sql_class.h b/sql/sql_class.h
> > index 18ccf992d1e..9e4bb1d4579 100644
> > --- a/sql/sql_class.h
> > +++ b/sql/sql_class.h
> > @@ -5475,7 +5476,13 @@ class select_dump :public select_to_file {
> >
> > class select_insert :public select_result_interceptor {
> > public:
> > + select_result* sel_result;
> The * should be next to the variable name. Not everybody does this, but the
> code around it does, so let's be consistent.
> >TABLE_LIST *table_list;
> > + /*
> > +Points to the table in which we're inserting records. We need to
> save the
> > +insert table before it is masked in the parsing phase.
> > + */
> > + TABLE_LIST *insert_table;
> This sort of code and comment is usually a bad idea:
> 1. We are adding an extra member, because of a hack in a different part of
>the code.
> 2. We are mentioning a specific flow for which this member is good for.
> This
>already begins to hint at a bad design decision. Sometimes there's no
> way
>around it, but this guarantees lack of re-usability. If, in the future
>we fix the "masking in the parsing phase" to no longer mask the first
> table,
>which we probably will do, because we have to to allow INSERT within
> other
>queries, the comment will become outdated. One needs to remember this
> too.
>
>Forgetting to update comments happens easily. Here is one example:
>One checks the class initially, finds the appropriate member to use,
>uses it in a different context than the original author thought of. The
>class then is no longer changed so it does not show up during review
> time,
>the reviewer doesn't think to check the class member's definition
> either.
>You now have outdated comments.
>
>To prevent this from happening, avoid as much as possible to explain
> where
>a member is used, rather explain what type of data it is supposed to
> store.
>The first sentence accomplishes that goal.
> >TABLE *table;
> >List *fields;
> >ulonglong autoinc_value_of_last_inserted_row; // autogenerated or not
> > @@ -5484,7 +5491,7 @@ class select_insert :public
> select_result_interceptor {
> >select_insert(THD *thd_arg, TABLE_LIST *table_list_par,
> > TABLE *table_par, List *fields_par,
> > List *update_fields, List *update_values,
> > - enum_duplicates duplic, bool ignore);
> > +
[Maria-developers] Coding Style Review INSERT RETURNING
Hi Rucha!
This is going to be a very thorough review looking at coding style. I'll
nitpick on every line of code where I find something, just be patient and
let's clean up everything.
You might consider me picking on the coding style a bit pedantic, but it
really is important. It is important because it helps in searching, it helps
in understanding the code faster, it helps to look-up history faster.
The good part is that once you get used to writing things properly, you won't
even have to think about it actively. It will just come naturally.
I suspect you do not have your editor configured to show whitespaces and
show you tabs versus spaces. Please look into your editor's settings to
highlight both tabs and spaces.
There are scripts that remove trailing whitespaces for you, across whole files.
I don't recommend them for MariaDB code as we have remnants from MySQL times of
these and because we care more about preserving the immediate history for
running `git blame`, we did not fully fix all files. For any other project, I
highly recommend you use them.
Some of these comments might slightly contradict what I said before. That is
because I wanted us to get things working first and I didn't want to burden
you with too many details initially. I'll look to explain myself now clearly
so you understand why some things are bad ideas. I want you to take this
review also as a general guideline for any other code you write, anywhere.
With these hints, tricks, ideas, you'll produce much higher quality code in
the long run. It will take you longer to write sometimes, but it really makes
a difference. :)
Also, you may have noticed some parts of our code do not agree with my general
guidelines. You have to take into account that this code was written over the
course of many years, by different people, with different levels of experience
or available time to implement something. Try not to make use of it as an
excuse for new code that you write. It's up to you to always write the best
possible version *with the time allowed*. We have adequate time now so we
can afford to look for better solutions.
With all that said, this took me a while to write and I may still
have missed a few things. Feel free to point out anything that looks strange.
Now, let's get to the review.
>
> diff --git a/sql/sql_class.h b/sql/sql_class.h
> index 18ccf992d1e..9e4bb1d4579 100644
> --- a/sql/sql_class.h
> +++ b/sql/sql_class.h
> @@ -5475,7 +5476,13 @@ class select_dump :public select_to_file {
>
> class select_insert :public select_result_interceptor {
> public:
> + select_result* sel_result;
The * should be next to the variable name. Not everybody does this, but the
code around it does, so let's be consistent.
>TABLE_LIST *table_list;
> + /*
> +Points to the table in which we're inserting records. We need to save the
> +insert table before it is masked in the parsing phase.
> + */
> + TABLE_LIST *insert_table;
This sort of code and comment is usually a bad idea:
1. We are adding an extra member, because of a hack in a different part of
the code.
2. We are mentioning a specific flow for which this member is good for. This
already begins to hint at a bad design decision. Sometimes there's no way
around it, but this guarantees lack of re-usability. If, in the future
we fix the "masking in the parsing phase" to no longer mask the first table,
which we probably will do, because we have to to allow INSERT within other
queries, the comment will become outdated. One needs to remember this too.
Forgetting to update comments happens easily. Here is one example:
One checks the class initially, finds the appropriate member to use,
uses it in a different context than the original author thought of. The
class then is no longer changed so it does not show up during review time,
the reviewer doesn't think to check the class member's definition either.
You now have outdated comments.
To prevent this from happening, avoid as much as possible to explain where
a member is used, rather explain what type of data it is supposed to store.
The first sentence accomplishes that goal.
>TABLE *table;
>List *fields;
>ulonglong autoinc_value_of_last_inserted_row; // autogenerated or not
> @@ -5484,7 +5491,7 @@ class select_insert :public select_result_interceptor {
>select_insert(THD *thd_arg, TABLE_LIST *table_list_par,
> TABLE *table_par, List *fields_par,
> List *update_fields, List *update_values,
> - enum_duplicates duplic, bool ignore);
> + enum_duplicates duplic, bool ignore,select_result*
> sel_ret_list,TABLE_LIST *save_first);
This is an artifact of our long-standing code base. TABS mixed with spaces. :(
Let's clean up all of this select_insert's constructor. Do not keep the tabs,
we are modifying it anyway. Turn all these tabs to proper spaces. Make sure
the function arguments match the beginning parenthesis like so:
