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.