Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

fix: toupper is not a member of std #1020

Merged
merged 3 commits into from
Jan 23, 2024

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

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
29 checks passed
@carlocorradini carlocorradini deleted the to_upper branch January 23, 2024 14:09
# for free to join this conversation on GitHub. Already have an account? # 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