Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

A cleaner public API #18

Closed
manxorist opened this issue May 21, 2021 · 2 comments · Fixed by #19
Closed

A cleaner public API #18

manxorist opened this issue May 21, 2021 · 2 comments · Fixed by #19

Comments

@manxorist
Copy link
Contributor

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant