Use library strncasecmp if available#535
Conversation
Introduces new flags regarding the availability of strncasecmp (which nowadays should be in strings.h), moves the custom function into its own sourcefile which is only compiled and connected to the rest of the code via a wrapper header. Closes goatshriek#531
|
There's one "FIXME" in my code, I haven't checked out the thread safety of the custom function (which I didn't write, I just refactored 😁 ). Also, I have tested this on Linux, but not on Windows, so somebody might want to check if it still compiles on Windows (it should). |
|
My apologies for the delay, I am aiming to review this and provide comments on the next 24 hours. |
goatshriek
left a comment
There was a problem hiding this comment.
Thanks for putting this together! It looks like there are some failing test cases, which I didn't look to closely at as I expect you'll be able to work out the issues. The other comments should be straightforward to address, and then I believe this will be good to go!
|
I have now gone through your code review and made the requested changes. The tests failing are not really related to my code (and they failed before I touched it), and as far as I can see it's because of three factors (which are out of scope for this pull request):
|
goatshriek
left a comment
There was a problem hiding this comment.
Thanks for digging into that test! The implementation is indeed broken, as you outlined. I'll fix this in a separate change - if you don't mind, please comment out the failing test case, and I will re-enable it when I fix the issue. I'd like to see the rest of the CI tests pass to make sure they're good to go.
Otherwise, I believe this will be good to go once the header path is updated in src/config/no_strncasecmp.c, and the header check issues are resolved. The header checks should be resolved by adding entries into the tools/check_headers/stumpless_private.yml manifest for the new symbols HAVE_STRNCASECMP, config_strncasecmp and strncasecmp_custom, and an entry into tools/check_headers/standard_library.yml for strncasecmp.
|
Ok, after disabling the test the ones running on my fork are complaining about not being able to upload coverage reports. But more important: the Windows build returns success (because it doesn't include coverage), so I guess we're good to go? |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## latest #535 +/- ##
=======================================
Coverage 90.82% 90.82%
=======================================
Files 47 47
Lines 4576 4566 -10
Branches 609 605 -4
=======================================
- Hits 4156 4147 -9
Misses 276 276
+ Partials 144 143 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Great, looks good! You don't have to worry about the coverage reports. The only remaining thing is to add the header check entries outlined above, which should resolve the header check that is currently failing in the static analysis. |
|
Ok, hopefully I ticked all the boxes and dotted all the i-s, it seems the only thing breaking now - at least in my fork - is "Sonarcloud Analysis" (which can't "find the project", whatever that means). |
|
Excellent work, thanks once again for putting this change together! I especially appreciate that you checked the CI runs in your fork while working through the fixes - this is a huge time saver and is often missed by contributors. |
|
Thanks a lot for being so patient with me, I might be somewhat useful at C (and maybe a little bit of C++) but not really into pull requests and what is expected. Looking at the CI results in my own fork was no biggie here, I've seen projects where it is more of an issue to understand what is going on (openssl for instance has a CI run that takes multiple hours). |
Introduces new flags regarding the availability of strncasecmp (which nowadays should be in strings.h), moves the custom function into its own sourcefile which is only compiled and connected to the rest of the code via a wrapper header.
Closes #531