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

Replace NDEBUG with custom flags to avoid ABI issues #1796

Merged
merged 2 commits into from
Mar 24, 2024

Conversation

jslee02
Copy link
Member

@jslee02 jslee02 commented Mar 24, 2024

  • Replaced NDEBUG with custom flags to avoid ABI issues for ABI of the library depends on the value of NDEBUG? #1795.
  • Added DART_ prefix to avoid potential name conflict.
  • Consolidated custom flags for release mode into DART_BUILD_MODE_RELEASE to simplify, based on usage.
  • Included config.hpp whenever needed
  • Fixed typos

Resolves #1795


Before creating a pull request

  • Document new methods and classes
  • Format new code files using ClangFormat by running make format
  • Build with -DDART_TREAT_WARNINGS_AS_ERRORS=ON and resolve all the compile warnings

Before merging a pull request

  • Set version target by selecting a milestone on the right side
  • Summarize this change in CHANGELOG.md
  • Add unit test(s) for this change
  • Add Python bindings for new methods and classes

@traversaro
Copy link
Contributor

Thanks! I just noticed something, that anyhow I am not sure about. If I recall correctly, Debian packaging set the CMAKE_BUILD_TYPE to None (i.e. empty string) and then it sets the release compilation flags itself, for keeping consistency across packages. Probably it could make sense to ensure that the release bits get compiled even if CMAKE_BUILD_TYPE is empty? fyi @j-rivero @azeey that I guess are interested in this.

Another use case that I am currently not interested in (as I also use Ninja in Windows) but that it could be affected (and I did not realized this while doing my original suggestion of using the config.hpp, sorry!) are Cmake multiple-config generators, such as Visual Studio, Xcode where the CMAKE_BUILD_TYPE is not used to specify the build type, as the build type is not fixed at configuration time. I guess it is ok not to support them, but probably it is better to print a clear error in CMake.

@jslee02
Copy link
Member Author

jslee02 commented Mar 24, 2024

Thanks for pointing these things out!

I wonder if setting it to RELEASE by default sounds like a practical fix for the Debian scenario. It would ensure we don't miss out on crucial optimization flags for release builds, and I'm leaning toward trying this approach.

Detecting these at configuration time is tricky since they're designed to let you choose the build type later. One idea could be to check the generator used during the CMake configuration phase. If we can identify multi-config generators, we might either set a default build type or warn that this setup needs a specific build type to be chosen at build time.

@jslee02 jslee02 merged commit 8931e2c into release-6.13 Mar 24, 2024
0 of 2 checks passed
@jslee02 jslee02 deleted the 6.13/build_mode_flag branch March 24, 2024 23:34
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.

2 participants