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

sc_puff should not be exported #153

Open
StefanBruens opened this issue Jan 2, 2024 · 6 comments
Open

sc_puff should not be exported #153

StefanBruens opened this issue Jan 2, 2024 · 6 comments
Labels

Comments

@StefanBruens
Copy link

Currently, the sc_puff symbol is exported, even when it is completely unused (i.e. when using zlib).

  1. sc_puff.c should not be included in the library when using zlib
  2. sc_puff should never be exported from the shared library ("visibility hidden")
  3. The sc_builtin/puff.h header file should be removed from the sources (completely unused)
  4. The sc_puff.h header file should not be installed, and probably moved to the sc_builtin directory
@cburstedde
Copy link
Owner

Thanks for your comment! I'm referring to the latest develop branch, where puff.h is removed.

We are already taking care that sc_puff is the only symbol exported, so the namespace is clean.
What is wrong with always compiling and installing it? Does the function get in the way in any way?

@StefanBruens
Copy link
Author

Exported symbols should be kept at minimum. For most users it is just dead code, increasing the size of the binary - code section and symbol table.

@cburstedde
Copy link
Owner

Thanks again; there are two angles to this issue, as far as I can see.

  1. If sc_puff is just a fallback in case zlib is not found, it would be ok to never export it and only compile it as a replacement. In this case, I'm with you.
  2. If sc_puff is considered independently valuable, maybe to benchmark/test against zlib, or for any other purpose we're forgetting right here, the author of a library is free to make the choice to always compile. This is more where I am.

(There may be an in-between in that with zlib found, sc_puff itself may call zlib's uncompress function. I might go there but I also might not.)

So I'm willing to discuss any hard reasons that preclude sc_puff being standard. I'm not willing to discuss 1. vs. 2., if that's ok, since this is ultimately a matter of opinion and taste.

Thanks again for watching out and bringing up your concern!

@cburstedde
Copy link
Owner

Actually I'm playing with the feature-puff branch to test further suggestions of yours.

@cburstedde
Copy link
Owner

Actually I'm playing with the feature-puff branch to test further suggestions of yours.

So is this significantly better? See ac2f826..7966db0.

@cburstedde
Copy link
Owner

Hmm. With this change, sc_puff is an external symbol, which is compiled and supplied with a prototype only when zlib is not found. This makes the set of exported symbols dependent on the configuration, which may not be the best for consistency. Still leaning towards unconditionally compiling sc_puff. The question remains whether the prototype should be visible in installed .h files or not.

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

No branches or pull requests

2 participants