On Fri, 24 May 2013 14:22:31 -0000, Laurynas Biveinis wrote:
> Review: Needs Fixing
>
> - The patch would be OK, but I did find one issue which IMHO
> requires recommitting (although the test cycle could be
> skipped) or follow-up addressing (I don't have a strong opinion
> on recomitting vs follow-up): trx_rw_is_active_low is a
> bool-returning function that returns NULL in one of the code
> paths.
Right, good catch! Thanks for fixing it as a followup.
>
> - If we are recommitting, then in_trx_serial_list should be bool,
> not ibool as well, and true/false used instead of 0/1 for
> setting it, spurious newline at diff line 278. The row0sel.cc
> change is spurious as well?
>
I deliberately made it ibool to be consistent with in_ro_trx_list /
in_rw_trx_list (which is also new 5.6 code BTW). But OK.
> Random remark:
>
> - trx_descr_cmp() should be defined in trx0trx.ic, not .c? So
> then compiler can resolve the function pointer to inlining. It
> is also possible to use std::binary_search() in the brave new
> C++ world.
>
bsearch() is a library function, so no inlining is possible. Is it
different with std::binary_search()?
> Last and least absolutely random remarks:
>
> - I don't fully understand the header comment for
> trx_rw_get_active_trx_by_id() stating that the returned pointer
> must not be dereferenced unless lock_sys->mutex is held. But
> the function itself dereferences the pointer in
> trx_state_eq()? I see that all callers hold the
> lock_sys->mutex, thus it does not seem to be a real issue
> though.
>
There's actually an upstream problem with that code, but it only applies
to debug builds. Will report it separately.
> - Upstream started making heavy use of __attribute__ function
> annotations to cause extra warnings on misuse, especially
> nonnull, warn_unused_result. Maybe not bad idea for us well
> where it applies.
>
Hi Laurynas,
On Fri, 24 May 2013 14:22:31 -0000, Laurynas Biveinis wrote: is_active_ low is a
> Review: Needs Fixing
>
> - The patch would be OK, but I did find one issue which IMHO
> requires recommitting (although the test cycle could be
> skipped) or follow-up addressing (I don't have a strong opinion
> on recomitting vs follow-up): trx_rw_
> bool-returning function that returns NULL in one of the code
> paths.
Right, good catch! Thanks for fixing it as a followup.
>
> - If we are recommitting, then in_trx_serial_list should be bool,
> not ibool as well, and true/false used instead of 0/1 for
> setting it, spurious newline at diff line 278. The row0sel.cc
> change is spurious as well?
>
I deliberately made it ibool to be consistent with in_ro_trx_list /
in_rw_trx_list (which is also new 5.6 code BTW). But OK.
> Random remark: search( ) in the brave new
>
> - trx_descr_cmp() should be defined in trx0trx.ic, not .c? So
> then compiler can resolve the function pointer to inlining. It
> is also possible to use std::binary_
> C++ world.
>
bsearch() is a library function, so no inlining is possible. Is it search( )?
different with std::binary_
> Last and least absolutely random remarks: get_active_ trx_by_ id() stating that the returned pointer
>
> - I don't fully understand the header comment for
> trx_rw_
> must not be dereferenced unless lock_sys->mutex is held. But
> the function itself dereferences the pointer in
> trx_state_eq()? I see that all callers hold the
> lock_sys->mutex, thus it does not seem to be a real issue
> though.
>
There's actually an upstream problem with that code, but it only applies
to debug builds. Will report it separately.
> - Upstream started making heavy use of __attribute__ function
> annotations to cause extra warnings on misuse, especially
> nonnull, warn_unused_result. Maybe not bad idea for us well
> where it applies.
>
Yes, that's good stuff.
/Alexey