JSBSim-Team / jsbsim

An open source flight dynamics & control software library
GNU Lesser General Public License v2.1
1.22k stars 420 forks source link

fix: toupper is not a member of std #1020

Closed carlocorradini closed 3 months ago

carlocorradini commented 3 months ago

Fix #1019

carlocorradini commented 3 months ago

@bcoconni Done std::toupper is overloaded so there is the need to help the compiler choose the correct one. See https://stackoverflow.com/questions/7131858/stdtransform-and-toupper-no-matching-function

codecov[bot] commented 3 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Comparison is base (af7910e) 24.87% compared to head (71d7cf1) 24.87%. Report is 3 commits behind head on master.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #1020 +/- ## ======================================= Coverage 24.87% 24.87% ======================================= Files 168 168 Lines 18908 18908 ======================================= Hits 4704 4704 Misses 14204 14204 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

seanmcleod commented 3 months ago

I reran the github build process and no compiler errors now across all compilers and platforms.

Out of interest what are you using JSBSim for and is there a particular need to use the fairly old MSVC 2015 compiler?

carlocorradini commented 3 months ago

Awesome 😎 MSVC 14.0 (2015) is the minimum supported compiler version (ABI compatibility), therefore I started with it. JSBSim is fully compatible with MSVC 14.0 except for this minor fix (I've created a PR for others).

seanmcleod commented 3 months ago

After you added the #include <cctype> did the following original code making use of a lambda not compile with MSVC 2015?

        std::transform(str.begin(), str.end(), str.begin(),
                       [](unsigned char c){ return std::toupper(c); }
                      );
bcoconni commented 3 months ago

After you added the #include <cctype> did the following original code making use of a lambda not compile with MSVC 2015?

        std::transform(str.begin(), str.end(), str.begin(),
                       [](unsigned char c){ return std::toupper(c); }
                      );

I was about to make the exact same point: the current code in the master branch just works. It's fine to add #include <cctype> if it is needed for VS2015. But I'm struggling to see the point in replacing a lambda function by a function pointer conversion. Both of them look equally ugly to me so let's stick with the current code.

And if it's done for the sake of C++ pedantry then I'll argue that you must use static_cast<> for type conversion 😉

carlocorradini commented 3 months ago

Done. It works! 🥳🤯 Let's see the workflows...