Skip to content

FromBytes family cleanups#307

Open
oherrala wants to merge 4 commits intojbaublitz:mainfrom
oherrala:slice-access
Open

FromBytes family cleanups#307
oherrala wants to merge 4 commits intojbaublitz:mainfrom
oherrala:slice-access

Conversation

@oherrala
Copy link
Contributor

Cleanups to basic types implementing FromBytes, FromBytesWithInput, FromBytesWithInputBorrowed.

  • Fix &str implementation of FromBytesWithInputBorrowed to handle string as null terminated. This might have been a bug? Now it also validates proper null termination without interior null bytes.
  • Change aforementioned &str reader to use &[u8]'s FromBytesWithInputBorrowed to access buffer
  • Change String reader to use &str's FromBytesWithInputBorrowed
  • Minor touch up to Vec<T>'s reader for consistent style
  • Change methods input parameter to consistently be Self::Input since some were usize
  • Add more tests

Add null termination checking for str's FromBytesWithInputBorrowed
implementation. The check also ensures there's no interior null bytes
in the string.

While here change to use more safe pattern for indexing the underlying
slice.

With this change it's now possible to implement String's
FromBytesWithInput by reading str first and then making it owned
reducing code duplication.
…entation

And then use &[u8] FromBytesWithInputBorrowed in str's
FromBytesWithInputBorrowed.
@jbaublitz
Copy link
Owner

@oherrala I believe this is addressing the problem of the null terminator being included in the String, is that correct? If so, I believe this does not qualify as a bug fix and needs to target v0.8.0 unfortunately. It's not causing an error or failures and I don't want to break anyone who may have already worked around this unfortunate bug in the code.

@jbaublitz jbaublitz added this to the neli-0.8.0 milestone Dec 4, 2025
@jbaublitz jbaublitz self-requested a review December 4, 2025 15:43
@jbaublitz jbaublitz added the bug label Dec 4, 2025
@jbaublitz
Copy link
Owner

@oherrala If you're okay with it, I'm going to pull these commits into #294.

@oherrala
Copy link
Contributor Author

@jbaublitz Sure, go ahead!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants