Skip to content

fix: toupper is not a member of std#1020

Merged
seanmcleod merged 3 commits intoJSBSim-Team:masterfrom
carlocorradini:to_upper
Jan 23, 2024
Merged

fix: toupper is not a member of std#1020
seanmcleod merged 3 commits intoJSBSim-Team:masterfrom
carlocorradini:to_upper

Conversation

@carlocorradini
Copy link
Contributor

Fix #1019

Copy link
Member

@bcoconni bcoconni left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As @seanmcleod already said, the code currently in master successfully compiles on all supported platforms (MSVC 2019 & 2022, MinGW, Mac OSX and Linux) while yours fails on all platforms but MSVC.

So I'd suggest you revisit this patch so that it compiles everywhere (and please no #ifdef).

@carlocorradini
Copy link
Contributor Author

@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
Copy link

codecov bot commented Jan 22, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

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
@@           Coverage Diff           @@
##           master    #1020   +/-   ##
=======================================
  Coverage   24.87%   24.87%           
=======================================
  Files         168      168           
  Lines       18908    18908           
=======================================
  Hits         4704     4704           
  Misses      14204    14204           

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

@seanmcleod
Copy link
Member

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
Copy link
Contributor Author

carlocorradini commented Jan 22, 2024

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
Copy link
Member

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
Copy link
Member

bcoconni commented Jan 22, 2024

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
Copy link
Contributor Author

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

@seanmcleod seanmcleod merged commit 6d126ec into JSBSim-Team:master Jan 23, 2024
@carlocorradini carlocorradini deleted the to_upper branch January 23, 2024 14:09
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.

MSVC: toupper is not a member of std

3 participants