Re: [PATCH] bindat strz fixes

[Stefan Monnier]

> I don't have a concrete need for it at the moment. I just noticed the bug
> while I was fixing the other bugs. I can leave that change out of the patch
> series if that is the only thing impeding merge.

I've installed the rest of the patch series into `master`, thanks.

>> The particular reason to prefer the current behavior is
>> backward compatibility (which we could call "inertia").
> Anyone that needs the current behavior should be using `str N`, not
> `strz N`.

`str N` does not give the same semantics when unpacking.

> If they're using `strz N`, then I would consider that to be a bug in
> their code.

It all depends how the format was defined.  "NUL-terminated N-bytes-max
strings" can be like the one currently supported or like the ones your
code supports.  Which one is better is irrelevant when the format is
not of your choosing.

I'm not at all opposed to supporting the kind of NUL-terminated strings
you propose but it should be *in addition* to the ones
already supported.

> If this change breaks someone, they can fix it easily: just
> change `strz N` to `str N`.

No, because unpacking "a\0" with (str 2) give "a\0" rather than "a".

> I understand that we should endeavor to maintain compatibility, but
> keeping the current behavior would be intentionally preserving a bug
> to accommodate other bugs.  I don't think that's a good trade-off.

It's only a bug if it doesn't match the format you need to use.
Your code introduces a bug when the format is defined to behave like the
current code :-(

>> Note also that `strz` without a length (or with a nil length) behaves
>> the way you want.
> No -- strz without a length results in variable length encoding. Without
> these changes, the only way to get a fixed length output with guaranteed
> null termination is to do `(substring str 0 (1- N))` before packing. (Or
> explicitly pack a separate zero byte immediately after the `str N-1` or
> `strz N-1` entry.)

...or define a new type.

>>>>        (bindat-unpack spec "ab")
[...]
> Regardless of what other applications produce, packed strings like that are
> invalid according to the spec provided to `bindat-unpack`.

Where in the spec/doc of Bindat do you see that "ab" is not a valid
input to `bindat-unpack` for `strz 2`?

> A real example of why it might be undesirable to attempt to process
> a strz string without a null terminator:

You don't need to convince me that the format you propose is useful.


        Stefan