Codecov Report

❌ Patch coverage is 72.72727% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 24.85%. Comparing base (b005a91) to head (c0417ef).
⚠️ Report is 5 commits behind head on master.

@@            Coverage Diff             @@
##           master    #1317      +/-   ##
==========================================
- Coverage   24.87%   24.85%   -0.02%     
==========================================
  Files         169      169              
  Lines       19655    19649       -6     
==========================================
- Hits         4889     4884       -5     
+ Misses      14766    14765       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Looks good.

The NDEBUG define with it's negative logic always make me double check it's use 😉

The NDEBUG define with it's negative logic always make me double check it's use 😉

The same with me 😄

BTW does MSVC define the flag NDEBUG when compiling with RelWithDebInfo ? I am asking because the Python wheels that are uploaded to PyPI are currently compiled with RelWithDebInfo:

Hmm, in terms of NDEBUG.

https://learn.microsoft.com/en-us/cpp/preprocessor/predefined-macros?view=msvc-170&redirectedfrom=MSDN

The compiler supports these predefined macros specified by the ISO C99, C11, C17, and ISO C++17 standards.

So it doesn't say it supports all macros specified by ISO standards, just 'these', and I don't see NDEBUG listed.

Also this post - taglib/taglib#790 mentions Visual Studio not defining NDEBUG in release builds.

And from Stackoverflow:

https://stackoverflow.com/questions/2290509/debug-vs-ndebug

Is NDEBUG standard?

Yes it is a standard macro with the semantic "Not Debug" for C89, C99, C++98, C++2003, C++2011, C++2014 standards. There are no _DEBUG macros in the standards.

I rely on NDEBUG, because it's the only one whose behavior is standardized across compilers and implementations (see documentation for the standard assert macro). The negative logic is a small readability speedbump, but it's a common idiom you can quickly adapt to.

Hmm, so this person is claiming it is standardized across compilers, but maybe they've never used MSVC? I see they also complain about the negative logic 😉

Having said all that, I'm fairly familiar with NDEBUG and 99% of my C++ compiler use over the years has been with MSVC.

Looking at the JSBSim vcxproj (as opposed to CMAKE) I see NDEBUG is defined for release build. So maybe the MSVC compiler doesn't automatically define it based on ISO standards, but maybe the default vcxproj that Visual Studio generates adds NDEBUG define by default?

Hmm, so this details CMAKE's Build Types.

https://gist.github.com/MangaD/475b8b413aff7682b803fb007083fb5c

Now it only lists compiler flages for GCC/Clang, and interestingly it explicitly defines NDEBUG for the release builds, i.e. it doesn't appear to rely on the compiler itself to define it?

So we need to figure out what compiler flags are passed to MSVC for CMAKE's RelWithDebInfo.

Please note that I have updated the PR and removed 2 templates of the Tiemethods:

• template <typename T> void Tie (const std::string &name, T (*getter)(), void (*setter)(T) = nullptr)
• template <typename T> void Tie (const std::string &name, int index, T (*getter)(int), void (*setter)(int, T) = nullptr)

They are not used anywhere and this will reduce the number of lines in FGPropertyManager.h (or should I say compensate the lines added by the PRs mentioned in this PR description).

So are we ready to merge this PR ?

I will address the NDEBUG flag for Windows in a separate PR.

So are we ready to merge this PR ?

Yep, I thought the only outstanding thing was the potential issue with NDEBUG on MSVC builds. Which you mention now you'll address in a separate PR.

Yep, I thought the only outstanding thing was the potential issue with NDEBUG on MSVC builds. Which you mention now you'll address in a separate PR.

Indeed, as you mentioned, we are already setting it up correctly in the MSVC project files and, according to GitHub Copilot, NDEBUG is defined by default for CMake when buidling with RelWithDebInfo and Release. But well that's AI which may or may not be trusted. So I guess we need to specify explicitly the flag in CMakeLists.txt for non-debug builds to be on the safe side, and also to protect ourselves against a change of CMake defaults.

Yep, setting NDEBUG ourselves explicitly in CMakeLists.txt is the safest option, at worst it will be defined twice in the arguments passed to the compiler.

When I initially took a look I thought I'd be able to fairly easily find a reference to some config file in the CMake installation listing the compiler options per compiler per build type, but wasn't able to at the time until I ran out of steam.

I did come across this link which mentions how you can get CMake to output the compiler options.

https://stackoverflow.com/questions/24767450/using-cmake-how-do-you-determine-the-default-compiler-flags-per-build-type

Green light to merge?

Looks good! Go for it

And for future reference the topic about NDEBUG is addressed in the PR #1330.