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

Improvements: Various User Focused Additions #217

Merged
merged 1 commit into from
Sep 23, 2024

Conversation

thearchivalone
Copy link
Contributor

@thearchivalone thearchivalone commented Sep 23, 2024

Most of my changes should be pretty self-explanatory through naming or notes that I added, but here's some extra clarification for a small handful:

  1. Dividing up the options a bit in the top level Cmake file and reordering some of them should improve the readability and flow a bit.
  2. Setting up DEBUG and INFO definitions based on build type won't fully reduce the terminal output when building for production/Release (SAIL_TRY function macros in status.h would probably need some tweaking to fully suppress them). NOTE: Release build will have some unused variables when compiling with Clang 17+ (not sure about GCC) but they shouldn't be a problem as long as the compiler doesn't convert them to errors in future releases (does happen from time to time).
  3. BUILD_TESTING doesn't fully fit everything else, so I changed that to BUILD_TESTS to fix that.
  4. Adding SAIL_ to the beginning of all options available for CMake that weren't built in did these things:
  • Added uniformity to them, giving a more professional feel. You want your software to be taken seriously and SAIL is your brand for it. :D
  • By setting them all that way, if your library is embedded in a project (like it is with mine), us as the users won't have to worry as much about setting options for SAIL affecting other embedded libraries. Example: by setting BUILD_SHARED_LIBS=ON in the Top-Level Cmake file, all other libraries would build the same way by default; that may not be the desired result (mixing Static and Shared libraries in a single project is not that uncommon, especially when making a cross-platform SDK of some type that needs both).
  1. If you ever decide to support other languages besides C and C++ in the future, building all of those bindings could be problematic for certain use cases, so having the ability to disable them and just build for C can be a time save if nothing else. There's also no point in setting CMAKE C++ Flags if only building for C, so don't print them out at the end to further hammer it home.

@thearchivalone thearchivalone force-pushed the usability branch 3 times, most recently from b53c003 to 0d3c032 Compare September 23, 2024 03:44
@HappySeaFox
Copy link
Owner

Hi! :) Thanks for the PR! I'll comment a little bit later on.

CMakeLists.txt Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
@HappySeaFox
Copy link
Owner

HappySeaFox commented Sep 23, 2024

SAIL_TRY function macros in status.h would probably need some tweaking to fully suppress them when building for Release

I'm not a big fan of introducing old-school DEBUG and INFO macro and disabling messages using them in debug/release mode. You can always disable noisy messages with sail_set_log_barrier(). I also think that codecs must use SAIL_LOG_TRACE more and not SAIL_LOG_DEBUG, I'll fix that. Update: f3bab2e

@thearchivalone
Copy link
Contributor Author

Thanks for the feedback. I'll get the binding stuff updated and will keep a personal fork for my needs. There's some other things I saw but they will only be useful to me based on this feedback. You have final say on what goes in and out of your library; this software is well-maintained and well-documented in the code, making it fun to just go through like I did last night. <3

@HappySeaFox HappySeaFox linked an issue Sep 23, 2024 that may be closed by this pull request
@HappySeaFox HappySeaFox merged commit 4aa3ede into HappySeaFox:master Sep 23, 2024
2 checks passed
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 this pull request may close these issues.

Introduce a CMake variable to disable bindings
2 participants