On Tue, Jun 23, 2020 at 01:55:32PM -0400, Denton Liu wrote:
> When we are dealing with a packet-line length header, we use the magic
> literal `4`, representing the length of "0000" and "0001", the packet
> line length headers. Use `strlen("000x")` so that we do not have to use
> the magic literal.
I'm not a huge fan of using strlen() for this, because it's _still_
magical (you cannot change "0000" in one place without changing it in
another"). And while it helps with understanding that "4" matches the
length of that string, IMHO it's harder to read because now I have to
make sure that those much longer strings all match up.
This refactoring also implies to me that if you changed all of "0000" on
one line you'd be fine, but that's emphatically not true. The magic
number "4" is used to size the buffer earlier in the function, and would
have to match (and of course since this is a network protocol, it's not
like you could even change those in isolation).
So I dunno. I kind of think the raw "4" is the most readable. It's quite
obvious to me in the context of a memcpy() what's going on. I don't mind
memcpy_literal() or similar that hides the repetition, but I think it's
hard to do here because of the arithmetic on the destination.
-Peff
