std::toupper(category.front()) should work

Thanks @Empact, I have addressed your feedback. The code to capitalize the first letter of a string has been isolated in void Capitalize(std::string& str) in utilstrencodings.cpp.

You can pass tolower directly like so: std::transform(net.begin(), net.end(), net.begin(), tolower);

The same doesn't work with std::tolower fyi.

You could cast like so, I believe that has the same effect: std::transform(net.begin(), net.end(), net.begin(), (int (*)(int)) std::tolower);

Note tolower accepts an int but handles it as an unsigned char: https://en.cppreference.com/w/c/string/byte/tolower

Thanks @Empact!

I opted for the lambda expression, because it allowed me to pass unsigned char characters to the tolower function.

I understood from the referenced documentation that it doesn't handle the int as an unsigned char (character to be converted. If the value of ch is not representable as unsigned char and does not equal EOF, the behavior is undefined.).

I hope you like the new implementation better.

Use BOOST_CHECK_EQUAL(ToLower('@'), '@') (it gives a clearer diagnostic message in case of failure).

Thanks, addressed in 0234af5

That's a lot of code!

What about return (c >= 'A' && c <= 'Z' ? (c - 'A') + 'a' : c);?

Thanks @sipa,

I asked myself the same question and I am happy to change it!

Just to share why I decided to use a mappings array:

Support for extended ASCII

Should there be a time where it becomes important to have support for characters in the extended ASCII range then it will be trivial to implement it. It will then just be a matter of adjusting entries in mappings array vs extending the implementation with control statements like if (c == 'Ü') return 'ü'; or alternative logic.

Performance

I wanted our locale independent functions to be a good citizen in terms of performance for code that requires it.

I did some naive tests to get an impression what the code would compile to and it seemed that the compiler was able to generate more efficient code in the case return mappings[c];

At all optimization levels I would get a pointer to the mappings array where mappings are performed on:

lea rax, [rip + __ZZ7ToLowerhE15tolowermappings]
movzx   eax, byte ptr [rdi + rax]

Alternatively in the case of (c >= 'A' && c <= 'Z' ? (c - 'A') + 'a' : c) the compiler would generate something like this at O3 optimization:

    mov eax, edi 
    add al, -65 
    cmp al, 26
    jae LBB3_2
    add dil, 32
LBB3_2:
    movzx   eax, dil

Particularly in the case of iterations I felt the compiler was able to do better optimizations, for example minimizing iterations by operating in sequences. In the case of the former it could do 4 mapping operations per sequence; while in the latter of at most two mappings per iteration.

On this basis of this I assumed the code would also perform better, but I realize I can be very wrong because I am no expert at this.

I hope this feedback is useful. It it isn't, I will gladly change the code!

I strongly prefer @sipa:s more readable shorter version. I'd bet any potential performance difference is insignificant in the grand scheme of things :-)

Thanks @practicalswift, will update the code per @sipa's feedback!

I agree with @sipa. If we need to add extended ascii support, we can address that then, and this is not a performance-critical operation that calls for extra code to optimize it. Simple, repetitive code adds visual noise and can hide bugs as well.

Thanks @Empact, I will update the pull request!

Thanks again for the feedback. I have updated the pull request and addressed your comments in 53b3bfa.

I feel it's slightly confusing to have a version of ToUpper that operates on chars (and does not modify the argument) while the version operating on vectors does modify. Perhaps call this ConvertToUpper or so?

Thanks, addressed in 0234af5

nit: I would return the string from Capitalize and inline that into the string construction.

mu-nit: Ruby uses the terms Downcase and Upcase for these operations, which I like. Just a suggestion.

I don't think we should define ConvertToUpper if we're not using it. The construction can be trivially re-created from ConvertToLower if we need it, and in the meantime it's just dead code.

nit: constexpr

nit: constexpr

Please add some ToUpper(...) tests as well :-)

Thanks @practicalswift! I was a bit too eager when I removed ToUpper(std::string&) and its tests (facepalm). Fixed in dff017f.

Since this modifies the category in-place, I think this breaks the comparison logic, e.g. on line 189.
Sorry, I think my original nit was misguided.

Capitalize(…) (std::string Capitalize(std::string str)) does not modify category in-place :-)

Hi @Empact, we should be good. When I addressed your feedback, I did change the method signature from void Capitalize(std::string&) to std::string Capitalize(std::string), which passes the argument by value (category is copied into str).

Thanks, I misread - agree the argument is passed by value, so it's fine. 👍

lol, tÖr, that's like the Swedish version of Tor right?

Lol @laanwj, best review comment ever! I do hidden messages or easter eggs from time to time, but this isn't one of them 😂😂