storm-devs / storm-engine

Game engine behind Sea Dogs, Pirates of the Caribbean and Age of Pirates games.
https://storm-devs.github.io/storm-engine
GNU General Public License v3.0
845 stars 120 forks source link

Fix invalid lexicographical_compare #478

Closed Hammie closed 1 year ago

Hammie commented 1 year ago

Fixes a theoretical issue because of using lexicographical_compare with <=. This triggered a debug assertion in Visual Studio.

Replaced it with the C++20 three-way comparison, which is hopefully supported on all compilers.

q4a commented 1 year ago

@espkk CI got ERROR: Failed requirement 'directx/9.0@storm/prebuilt' from 'conanfile.py' Can you check it?

espkk commented 1 year ago

Does clean local build work?

espkk commented 1 year ago

Btw, don't we want to try implementing our char_traits instead?

Hammie commented 1 year ago

std::char_traits could also work, I'll try it

Hammie commented 1 year ago

I changed the code to use char_traits. @espkk Is this sortof what you had in mind?

espkk commented 1 year ago

I changed the code to use char_traits. @espkk Is this sortof what you had in mind?

This looks good. Do we still have tests for it to make sure they are also fine?

Now we can also do

using istring = std::basic_string<char, ichar_traits>;

and most importantly

using istring_view = std::basic_string_view<char, ichar_traits>;

and use it in most cases where we use those comparators.

I'm not sure if we want to update all the existing code since it's pretty stable, but probably we can gradually simplify it in this way

Hammie commented 1 year ago

We still have tests for iEquals and iLess. Didn't have to change those.

The aliases for istring and istring_view are also already in there.

Over time I think it would make sense to use those for our case-insensitive code and components. This will require us to do explicit conversions in between case-sensitive and case-insensitive code, but a more explicit border is probably a good thing.