Code review comment for lp:~vlad-lesin/percona-server/i_s-innodb-changed-pages

Revision history for this message
Vlad Lesin (vlad-lesin) wrote :

> Vlad -
>
> The code looks good, I have some other minor comments:
>
> Lines 461--462 and 464--465 should be exchanged for a bit
> better comment readability (IMHO).
>
Done.

> The function return value on line 588 is documented
> incorrectly. It should not be a separate line with an /*!<out:
> ... */ comment but rather a "@return desription" tag at the end of
> the header comment, grep InnoDB for "@return" for examples.
>
I saw the both styles. For example, see i_s_innodb_buffer_pool_pages_fill() and i_s_innodb_buffer_pool_pages_*() at all. So I just got one of them as a template. So as "@return" style is more readable I fixed it.

> I am not sure what to do about the comment block in 610--630. It
> is very descriptive and it is good to explain why the condition
> it describes must look the way it does. But it breaks the code
> flow and readability. Perhaps extract
> LOG_BITMAP_ITERATOR_START_LSN(i) <= max_lsn into a separate
> (static inline) function and make that comment its header comment?
I think this comment will be out of context. Could you look at the padded version of the comment please? Maybe it looks readable enough. If no I will replace it in separate function.

« Back to merge proposal