temisu/ancient

A cleaner public API

manxorist opened this issue · 2 comments

manxorist commented

This issue is a follow-up to things already partially discussed in #13 (3).

  1. I'd prefer a true facade-like public API that does not leak any implementation detail. The current implementation leaks Decompressor::Registry (and related functionality), and in particular also leaks all classes derived from Decompressor. In order for RTTI to work in client code (when ancient is compiled with RTTI (the current Makefile does not, however for best interoperability with client code, a shared library should be)), all classes derived from Decompressor would also need to have public visibility. I do not think this is feasible or desirable. As the currently exposed interface are base classes with a vtable, the size of the vtable becomes part of the ABI. This makes extending these interfaces (which are also used internally) difficult without breaking the ABI.
  2. I do not think Buffer needs to be exposed at all. I think in the public API, std::vectorstd::byte (or std::vector<uint8_t>) is all that is needed. For internal use, this can be wrapped in a Buffer at the API boundary for ease of use.
  3. Is OutOfMemoryError really needed? I think standard C++ std::bad_alloc has exactly the same meaning. OutOfMemoryError can be annoying for client code: Anything in the standard library will already throw std::bad_alloc in case of allocation failure, and ancient will itself throw OutOfMemoryError in the same situation. This means client code would need to always handle both, as ancient internally uses standard library features that throw std::bad_alloc (the obvious one being std::make_unique here).

I am currently working on this and will suggest a pull request when I am done.

manxorist commented

I have pushed my work-in-progress implementation to https://github.com/manxorist/ancient/tree/wip-api-facade. This is meant as a base for discussion and not yet as a pull request to be merged.

Also note that this is not yet tested.

This branch also already contains #14 and #15 merged.

manxorist commented

Given manxorist@4fc8a14, can we even get completely rid of void decompress(std::vector<uint8_t> &rawData,bool verify); with external allocation in the public interface? Or is there some intended use for such an interface that I am not aware of?