hexdump_pp: Respect indentation of parent boxes#175
Conversation
|
I appreciate this enhancement of the pretty printer! |
|
@hannesm suggested inserting whitespace after every 8 octets also, so a 16-octet line would become thus making it easier to spot "byte number 9." I'm happy to add that, but would like to hear the opinion of the maintainer(s) before introducing that in this PR. Thoughts? |
|
The extra white space also sounds good to me. We could also have a second column that prints the ascii printable characters as hexdump does, to make protocol dumps easier to parse. That doesn't need to block this PR of course. |
|
For more complete formating, I have no strong opinion on where this function could go, but it would be nice to not duplicate it too much :-) |
$ echo {A..z} | hexdump -C
00000000 41 20 42 20 43 20 44 20 45 20 46 20 47 20 48 20 |A B C D E F G H |
00000010 49 20 4a 20 4b 20 4c 20 4d 20 4e 20 4f 20 50 20 |I J K L M N O P |
00000020 51 20 52 20 53 20 54 20 55 20 56 20 57 20 58 20 |Q R S T U V W X |
00000030 59 20 5a 20 5b 20 20 5d 20 5e 20 5f 20 60 20 61 |Y Z [ ] ^ _ ` a|
00000040 20 62 20 63 20 64 20 65 20 66 20 67 20 68 20 69 | b c d e f g h i|
00000050 20 6a 20 6b 20 6c 20 6d 20 6e 20 6f 20 70 20 71 | j k l m n o p q|
00000060 20 72 20 73 20 74 20 75 20 76 20 77 20 78 20 79 | r s t u v w x y|
00000070 20 7a 0a | z.|
00000073I don't see an obvious way to do the index and have it look nice in the face of line-wrapping since the The functions currently look like this: val Cstruct.hexdump_pp : Format.formatter -> t -> unit
val Cstruct.hexdump : t -> unitI'd propose changing it to something like (happy to change the names and/or interface): val Cstruct.hexdump_pp : ?print_offset:bool -> Format.formatter -> t -> unit
(** Pretty-print octets in hex, optionally prefixed by the hexadecimal offset for each line *)
val Cstruct.hexdump_ascii_pp : ?print_offset:bool -> Format.formatter -> t -> unit
(** Pretty-print both hex octets and printable characters, like hexdump -C *)
val Cstruct.hexdump : ?print_offset:bool -> ?print_ascii:bool -> t -> unitSuggestions welcome! |
|
@cfcs I like optional arguments more than a different function -- @samoht I also feel bad that there's code duplication between maybe hex should even be integrated into Cstruct? I commonly have (esp. for testcases) local definitions for |
|
@hannesm I'm fine with optional arguments, I'll write a proposal for the ASCII stuff and update this PR. @hannesm @samoht : Re: Possible additions (slightly off-topic):
(* find char [c] in [b], starting at [offset] and giving up after [max_offset] *)
let index_opt b ?(max_offset) ?(offset=0) c : int option =
(* find substring [needle] in [b] *)
let find b ?(max_offset) ?(offset=0) needle =
let split_by_char c ?offset ?max_offset buf : (t*t, 'error) result =
|
|
It's clearly becoming out of scope of this PR, but yea, |
|
Ok, I think I bit of more than I can chew... After spending 16-20 hours on trying to understand |
|
then let's merge this now, or is anyone against this change (I'm too shy to press merge buttons)!? |
|
I feel your pain about understanding |
This wraps
hexdump_ppoutput in a box, making it respect the parent box' indentation:the old behavior was to output:
I experimented with getting rid of the
if i mod 16 = ...block by usingFormat.pp_set_margin, but ran into this (bug?): https://gist.github.com/cfcs/b39f0fa96a96235e41ff382eca8c3b86Edit: It also changes the behavior when the lines are too small to print 16 octets to this (I couldn't see an obvious way to determine how many chars are available in the current line to print the same amount of octets on all lines, but I feel this is more readable in any case):
The old behavior was:
ping @avsm