maria-developers team mailing list archive

[Jeremy Cole <jeremycole@xxxxxxxxxx>]

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. :(

Regards,

Jeremy