rikyoz / bit7z

A C++ static library offering a clean and simple interface to the 7-zip shared libraries.
https://rikyoz.github.io/bit7z
Mozilla Public License 2.0
623 stars 113 forks source link

Instantiating bit7z::Bit7zLibrary as part of a class member #28

Closed kenkit closed 4 years ago

kenkit commented 4 years ago

Describe the bug Hi, I can't figure out how to instantiate Bit7zLibrary since the class is uncopiable and unassignable.

Expected behavior I find this library similar to 7zip-cpp so I think it should be possible to load and initialize 7z.dll once in my class.

Environment details (put an x in all the boxes that apply):

Additional context Add any other context about the problem here.

rikyoz commented 4 years ago

Hi! First of all, thank you for submitting your issue!

Describe the bug Hi, I can't figure out how to instantiate Bit7zLibrary since the class is uncopiable and unassignable.

The problem is that there is little point in copying or assigning Bit7zLibrary objects. In fact, since they own a HMODULE (which points to the DLL base address), they are responsible both for loading and freeing it. Now, if a Bit7zLibrary object is created by copying another one, they will both have the same HMODULE, and if one of the two objects is destroyed, it will free its HMODULE, potentially unloading the DLL and making the HMODULE of the other object invalid!

However, there should be no problem in having a Bit7zLibrary object as class member and initializing it in the initializer list of the constructor. I don't know if it is enough for your use case, do you have any examples of code where it's not?

Expected behavior I find this library similar to 7zip-cpp so I think it should be possible to load and initialize 7z.dll once in my class.

Imho, the API provided by 7zip-cpp has some issues. For example, it forces the user to call the Load() function before using the library object, while the constructor does nothing. I think is a bad interface design, since it doesn't enforce a correct usage of the library object, i.e. using it only after it has loaded the DLL. Moreover, this allows the users to call Load() multiple times, which is unlikely (at least in any normal use case I can think of). Ultimately, it allows copying/assigning library objects, which I think is bad as I stated before.

kenkit commented 4 years ago

Thank you for the nice explanation, I will use the method you recommended by instantiating it in a constructor.

rikyoz commented 4 years ago

You're welcome!

kenkit commented 4 years ago

I now use it like this in my class

//header
 std::shared_ptr<bit7z::Bit7zLibrary> lib;

//Load 7z.dll
 lib=std::shared_ptr<bit7z::Bit7zLibrary>(new bit7z::Bit7zLibrary(sevenz_dll_path.c_str()));

//Member function
bit7z::BitCompressor compressor{ *lib, bit7z::BitFormat::Zip };

I wonder if this will be thread safe ? I will do some tests on this much later.