18.07.2013 19:39, Jeremy Cole пишет:

Sanja,

Re: https://mariadb.atlassian.net/browse/MDEV-4645

Re: http://lists.askmonty.org/pipermail/commits/2013-July/005093.html

I saw your commit on the commits list (sorry, wasn't on the list before the
mail went out, so I can't reply directly to that message).

A couple of points related to the commit:

    - You seem to have an error in the get_header_len, as it's implemented

    only in the base class, and you've left functions with the correct
    signature for it but the wrong names in the two frozen classes (they were
    left named has_fixed_minimal_header). This would mean the change is
    completely broken. :)

    - The change to get_header_len I think is in general fine, although it

    does alter the point of the whole thing -- there is again no way to exactly
    identify which events have frozen headers, programmatically (i.e. you can't
    differentiate them from events who just "happen" to have header_len =
    LOG_EVENT_MINIMAL_HEADER_LEN). I think that doesn't matter for this
    patch, but does defeat the point of what I was trying to add.

    - I don't really understand why you've made the hex printing code so

    complex and completely unreadable (IMHO). This is not performance-sensitive
    code and in my opinion should favor readability over performance. All you
    seem to have saved in this change is to avoid printing some static
    formatting characters ("#", "|", spaces, etc.) inside the loop. However, I
    can hardly even figure out what it's supposed to be doing now.

    - Additionally, you've changed the output slightly -- normally the ASCII

    character section prints its ending "|" immediately after the last
    character; it seems now to be fixed position (leaving extra spaces before
    "|". This differs from the output of hexdump, and it somewhat
    ambiguous/confusing.

    - You've mixed tabs and spaces. :(

Thank you a lot for this bug-report, I saw that the test suite do not cover the bug so did not pushed the fix hoping for your response.

Test suite still highly appreciated :) (if it is possible to make it at all, I doubts)

The cause of changes was:
1) avoid double copying string (formatting characters are not really matter) and extra memory allocation on the stack.
2) avoid 'if' in case when method can return value which we can use directly (looks better IMHO)
3) avoid extra virtual method call (they are quite expensive)
Yes 1 and 3 are about performance but I deal with server code mostly.

I'll change the patch, thank you yet another time.