Skip to content

Revisit `RESPValue` and `RESPValueConvertible` implementations.

Nathan Harris requested to merge cleanup-unsafe into 47-proposal-feedback

Motivation:

Johannes provided a fair code review of the project and summarized his findings in issue #48 (closed), and one of the prime offenders was all of the unsafe* APIs (pointers, buffers, bytes) that were used with RESPValue and RESPValueConvertible.

He also provided great feedback and pointed out good points of confusion with the API design of RESPValue and RESPValueConvertible.

Modifications:

  • Return to using Array instead of ContiguousArray for RESPValue.array storage
  • Update all documentation to be more thorough in explaining how the types should be used and conformed to.
  • Remove all uses of unsafe* APIs where possible
  • Change implementations to be a lot more type and memory safe, double checking assumptions
  • Remove conformance to ExpressibleBy*Literal as it is too easy for users to shoot themselves in the foot and saves only a few characters over .init(bulk:)
  • Create new RedisNIOTestUtils target for common test extensions, making them public
  • Move most almost all implementations of RESPValue computed properties into the RESPValueConvertible conformances

Result:

Users should be more safeguarded by the API against unknowingly getting incorrect RESPValue representations, the API design of RESPValue and RESPValueConvertible should be much clearer, and memory safety should be at a higher bar from these changes.

This resolves issues #55 (closed) & #48 (closed), and contributes to issue #47 (closed).

Edited by Nathan Harris

Merge request reports