RFC: Hide zlib and expat libs from the user of Poco libraries #4579
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
added 3 commits
June 7, 2024 20:31
Nothing use this and it is not even included in Visual Studio project files. Remove it so it will not confuse any more.
Hide zlib completly from user. This way we do not need to publish zlib.h or zconfig.h. As we now have two different pointer initalizing in constructor I choose to use unique pointers so it is more obvious those are safe. I also choose to use make_unique which default initalize z_stream_t. This makes code more readable as we do not need to specifie every field of z_stream_t. It really should not matter much if we initialize couple field for nothing. If does we should add comment about that. Still keeping _buffer without inializing as it is quite big.
Hide expat completly from user. This way we do not need to publish expat.h or expat_external.h. I move also headers to orignal locations so diff is smaller compared to original.
|
Please say if we example still want to expose ParserEngine. I will try to make necessary changes for that. I do not remember exactly if that would be easy as this has been quite long in my POC box. Also if this is path we will take I also need to check VS project files more closely. |
| @@ -104,7 +101,7 @@ class Foundation_API DeflatingIOS: public virtual std::ios | |||
| /// order of the stream buffer and base classes. | |||
| { | |||
| public: | |||
| DeflatingIOS(std::ostream& ostr, DeflatingStreamBuf::StreamType type = DeflatingStreamBuf::STREAM_ZLIB, int level = Z_DEFAULT_COMPRESSION); | |||
| DeflatingIOS(std::ostream& ostr, DeflatingStreamBuf::StreamType type = DeflatingStreamBuf::STREAM_ZLIB, int level = -1); | |||
There was a problem hiding this comment.
Going from Z_DEFAULT_COMPRESSION to just -1 is a loss of info for users. Can we add a POCO_DEFAULT_COMPRESSION const for this?
There was a problem hiding this comment.
There is an issue with other (non-default) compression levels whenever the user decided to go with non-default.
Shall Poco maintain a copy of compression values that are identical with the ones in zlib?
| #include "Poco/zlib.h" | ||
| #endif | ||
|
|
||
| struct z_stream_s; |
There was a problem hiding this comment.
Don't think this really needs to be here. Just do struct z_stream_s* _pZstr; in the class (unless I'm not seeing another usage here).
matejk
changed the title
matejk
reviewed
Jun 10, 2024
matejk
reviewed
Jun 10, 2024
| #include <zlib.h> | ||
| #else | ||
| // Quirk before we move zlib to external libs. | ||
| #include "../../src/zlib.h" |
There was a problem hiding this comment.
The path to zlih.h shall be added CMake's include path instead.
|
@teksturi , any updates on this? |
|
Replaced with #4724. |
matejk
added a commit
that referenced
this pull request
Oct 4, 2024
) (#4724) * foundation: Remove unused ucp.h Nothing use this and it is not even included in Visual Studio project files. Remove it so it will not confuse any more. * foundation: Hide zlib from user Hide zlib completly from user. This way we do not need to publish zlib.h or zconfig.h. As we now have two different pointer initalizing in constructor I choose to use unique pointers so it is more obvious those are safe. I also choose to use make_unique which default initalize z_stream_t. This makes code more readable as we do not need to specifie every field of z_stream_t. It really should not matter much if we initialize couple field for nothing. If does we should add comment about that. Still keeping _buffer without inializing as it is quite big. * xml: Hide expat and ParserEngine from user Hide expat completly from user. This way we do not need to publish expat.h or expat_external.h. I move also headers to orignal locations so diff is smaller compared to original. * chore(Foundation): Compression level constants --------- Co-authored-by: Kari Argillander <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Seperete zlib and expat so they are only expose internally. There should not be reason for user to use these.
This makes #4358 easier to implement as all external libraries are not exposed to end user.