search

ericsink / SQLitePCL.raw

A Portable Class Library (PCL) for low-level (raw) access to SQLite
Apache License 2.0
512 stars 106 forks source link

added sqlite3_serialize() and sqlite3_deserialize() (src) #536

Closed garthmz closed 7 months ago

garthmz commented 1 year ago

Opening this PR for discussion. I'm not sure about the approach, or if there are additional requirements to get a change like this accepted.

The raw additions seem heavy compared to its other methods, but I tried to take a similar stance of handling all memory management there. Since the sqlite flags for these functions are related to memory management, they are not exposed in the raw api. The support for Stream is a simple, standard way to avoid a buffer, but it is a new reference to System.IO.

ericsink commented 1 year ago

From an initial read, this is nice work. Thanks for submitting it. I may still have feedback or requested changes when I go through it more closely, which I hope to get done soon.

ericsink commented 1 year ago

Sorry for the long delay in reviewing this.

  1. In principle, I have to push back on the use of System.IO.Stream. Anything that high-level is usually omitted from this library, with the expectation that upper layers will provide it.

  2. I'm staring at the deserialize call that accepts a ReadOnlySpan and then allocates a block and copies it. That just feels wrong. I haven't actually used the sqlite3_deserialize() API, so I've been reading the docs, and I sort of see why you did things this way. But it raises questions as I look at the code.

  3. Once we get the code looking okay for merge, I'll be asking for this PR to include test cases.

ericsink commented 7 months ago

See #547