> - many lines exceed the 80 line width limit
> - it is a good practice to end replication test cases with "--source
> include/rpl_end.inc"
> - Yoda notation in comparisons ("0 == ...")
Noted.
> - variable declarations in the middle of a block is C99 (i.e. more
> Windows incompatibilities)
Isn't ha_innodb.cc C++? But I will fix them in any case.
> - unnecessary (char *) cast for the first argument in bzero()
> - I don't think we actually need IO_CACHE in
> innobase_do_overwrite_relay_log_info(). it's basically an enhanced
> version of fwrite() & friends, whereas we only want a single write
> of buff to the file.
> - no spaces around '=' sign in many places
> - no braces in single-statement if()s
Noted.
> - in general, I would suggest to fix all of the above by leaving the
> (incorrectly formatted) code alone and not moving it into a
> separate function. That would save me about half an hour on reading
> and commenting on changes that are not really changes but rather
> moving the code around, and allow me to focus on what's really been
> changed.
> - same goes for fname -> info_fname renaming
There are two places where I extracted new functions from existing code: in ha_innodb.cc and in trx0sys.c. Which one of them, or both, are you referring to? The trx0sys.c one I can revert, but IMHO it is easy to review too. Re. ha_innodb.cc changes, I have extracted innobase_do_overwrite_relay_log_info() because I needed to call it from another function as well and I don't see a good alternative to making a new function: if I copy pasted the code, I'd still need to adjust it heavily due to different local var context and the end result would be very close to innobase_do_overwrite_relay_log_info() anyway then. And I did the fname/pos variable rename & split, because these variables being repurposed five times is way beyond my pain threshold and extracted from innobase_setup() they stop working anyway.
Is there any way I can make the review easier with separate function? A separate MP with no functional changes perhaps?
Alexey -
Thanks for the review
> - many lines exceed the 80 line width limit rpl_end. inc"
> - it is a good practice to end replication test cases with "--source
> include/
> - Yoda notation in comparisons ("0 == ...")
Noted.
> - variable declarations in the middle of a block is C99 (i.e. more
> Windows incompatibilities)
Isn't ha_innodb.cc C++? But I will fix them in any case.
> - unnecessary (char *) cast for the first argument in bzero() do_overwrite_ relay_log_ info(). it's basically an enhanced
> - I don't think we actually need IO_CACHE in
> innobase_
> version of fwrite() & friends, whereas we only want a single write
> of buff to the file.
> - no spaces around '=' sign in many places
> - no braces in single-statement if()s
Noted.
> - in general, I would suggest to fix all of the above by leaving the
> (incorrectly formatted) code alone and not moving it into a
> separate function. That would save me about half an hour on reading
> and commenting on changes that are not really changes but rather
> moving the code around, and allow me to focus on what's really been
> changed.
> - same goes for fname -> info_fname renaming
There are two places where I extracted new functions from existing code: in ha_innodb.cc and in trx0sys.c. Which one of them, or both, are you referring to? The trx0sys.c one I can revert, but IMHO it is easy to review too. Re. ha_innodb.cc changes, I have extracted innobase_ do_overwrite_ relay_log_ info() because I needed to call it from another function as well and I don't see a good alternative to making a new function: if I copy pasted the code, I'd still need to adjust it heavily due to different local var context and the end result would be very close to innobase_ do_overwrite_ relay_log_ info() anyway then. And I did the fname/pos variable rename & split, because these variables being repurposed five times is way beyond my pain threshold and extracted from innobase_setup() they stop working anyway.
Is there any way I can make the review easier with separate function? A separate MP with no functional changes perhaps?