3
\$\begingroup\$

I am challenging myself to try to attempt to write a function that is as efficient, portable and failsafe as possible. The function is very simple and just converts a string (or wstring) to lowercase.

Are there any improvements you think I could make and any errors I have done?

#ifdef UNICODE
typedef std::wstring tstring;
typedef std::wstring::npos tnpos;
#else
typedef std::string tstring;
//typedef std::string::npos tnpos;  // throws compiler error?!
#endif


tstring toLower(const tstring& str)
{
    // Alternative 1: Returns a copy of a local string so not that efficient.

    unsigned int len = str.length();    // use unsigned int so I only have 4 bytes as opposed to 16 for int
    tstring lower(len, 'a');            // ensure string is correct size to avoid dynamic resizing. Reserve at construction; performing 2 steps at once defintion and reserving - is faster right?
    // or use lower.reserve(len);

    for (unsigned int i=0; i<len; i++)  // iterate using indexes. Maybe iterators could be more fail safe/efficient/etc.?
        lower[i] = tolower(str[i]);     

    return lower;
}

void toLower(const tstring& inStr, tstring& outStr)
{
    // Alternative 2: Have user define string and pass by reference (this would be faster?)

    unsigned int len = inStr.length();  // use unsigned int so I only have 4 bytes as opposed to 16 for int. Store length in local variable because we look it up again in the below for loop
    outStr.resize(len);                 // ensure string is correct size to avoid dynamic resizing.

    for (unsigned int i=0; i<len; i++)  // iterate using indexes. Maybe iterators could be more fail safe/efficient/etc.?
        outStr[i] = tolower(inStr[i]);
}
\$\endgroup\$
  • \$\begingroup\$ That typedef to npos should not compile, since npos is a static member of string/wstring, not a type. However, a typedef is completely unnecessary since you can just access it with your tstring::npos. \$\endgroup\$ – glampert Apr 14 '15 at 1:55
5
\$\begingroup\$

npos is not a type

typedef std::wstring::npos tnpos;

This should not be working. So I presume you have not tested this.

Don't assume the size of an int.

// use unsigned int so I only have 4 bytes as opposed to 16 for int

Neither of those are safe assumptions. Also those are the wrong types. If you don't know the type you can use auto

auto len = str.length();

You are making assumptions about types.

tstring lower(len, 'a');

If you are using std::wstring then 'a' is not the correct type (should be L'a'). You would need to make that a wide character. So you are probably looking at another macro to create the correct type.

Also the function tolower() is for use with char (not wide char). There is a wide character variant called towlower() so you may need to use another macro for that.

Most functions are in std::

lower[i] = tolower(str[i]);

Either you have a using namespace std; (which is bad). Or you have included the C header file for tolower() (<ctype.h>) (which is icky but OK) or you have included the C++ header file (<cctype>) (yay) and are getting lucky (as it is implementation dependent if this is in the global namespace for C++ (Note: it is always in the std:: namespace)).

RVO and NRVO and move semantics.

// Alternative 2: Have user define string and pass by reference (this would be faster?)

No this should not be quicker (but always time things).

NRVO (Named Return value optimization). The old C++03 compilers can easily see this as a value that is not used in the construction function and will actually build the string in place at the destination (rather than build it locally and copy it out).

You can test this by rigging up your own version of a string and seeing if the copy constructor is called when you return it (if you have optimizations on it does not).

With C++11 we introduce move semantics. The return value of a function is an r-value reference (may be some other complex names for it). Basically if the compiler can not use NRVO/RVO then it can use move semantics to move the object out of the function (rather than copying it).

Short story don't bother to pass output parameters.
It is much more logical to use return values. The compiler is going to do a bunch of optimizations that make it as affective (if not more so).

Better approach.

Try and use algorithms:

// In place to-lower.
std::transform(std::begin(str), std::end(str),
               std::begin(str), std::tolower);

// To another string (Not pre-sized)
std::transform(std::begin(str), std::end(str),
               std::back_inserter(dst), std::tolower);

// To a pre-sized string
std::transform(std::begin(str), std::end(str),
               std::begin(dst), std::tolower);
\$\endgroup\$
5
\$\begingroup\$
typedef std::wstring::npos tnpos;

std::string::npos and std::wstring::npos are static constant integer members of std::string and std::wstring, respectively. They are not types and they cannot be typedef'd.

unsigned int len = str.length();

This is incorrect. std::string::length() has a return type of size_t std::string::size_type which may be larger than an unsigned int depending on your platform and compiler. Other class methods will use the same type for length/position/indexing, so you should make this change for all the other uses of unsigned int.

// or use lower.reserve(len);

No, please don't. That reserves space for the string, but it does not resize it. There is a resize() method.

Since you said you're able to use C++11 features, as long as they're enabled, the first version of the function (the one that returns a string) will work without copying. It will use the move constructor. So you don't have to use the cumbersome version with an input and output parameter. For more information see http://blog.smartbear.com/c-plus-plus/c11-tutorial-introducing-the-move-constructor-and-the-move-assignment-operator/

\$\endgroup\$
  • 1
    \$\begingroup\$ Specifically, std::string::size_type. \$\endgroup\$ – Jamal Apr 14 '15 at 2:37
  • \$\begingroup\$ heh, my docs must not be following the STL then \$\endgroup\$ – Snowbody Apr 14 '15 at 2:42
  • \$\begingroup\$ @Snowbody thanks for the advice. Yes I can use C++11 or 14 so I'd be interested in those. \$\endgroup\$ – sazr Apr 14 '15 at 4:11
3
\$\begingroup\$

Are there any improvements you think I could make and any errors I have done?

Yes:

#ifdef UNICODE
typedef std::wstring tstring;
#else
typedef std::string tstring;
#endif

If you want two versions of the code, consider using templates (instead of conditioning the code to the UNICODE macro): you will only compile once, and without imposing flags on the compiler/build system/ide.

tstring toLower(const tstring& str)
{
    // Alternative 1: Returns a copy of a local string so not that efficient.

Actually, it is very efficient, because it uses move construction and/or ellision (returning by value shouldn't cost much - if anything, since c++11).

    unsigned int len = str.length();    // use unsigned int so I only have 

Size of a string cannot be measured accurately by an unsigned int (at least not portably or in all cases).

Safe alternatives:

    auto len = str.length(); // 1
    decltype(str.length()) len = str.length(); // 2
    std::string::size_type len = str.length(); // 3

That said, you should only need one retrieval of the size in this function, and that is for preallocating the result (so you need no len variable at all):

    tstring lower(str.length(), 'a');

Do not iterate using indexes, unless you need to keep track of the index for another operation. Iterators are more idiomatic and safer - because it is more difficult to make a mistake).

That said, you don't need iterators either - you need std::transform.

    using std::begin; using std::end; using std::transform; using std::tolower;
    transform(begin(str), end(str), begin(dst), tolower);
}

The second function should have similar changes.

Final code for first function you wrote (with no dependence to unicode and all my suggested changes):

template<typename C, typename T, typename A>
std::basic_string<C,T,A> toLower(const std::basic_string<C,T,A>& source)
{
    decltype(toLower(source)) result( source.size(), ' ' );

    using std::begin; using std::end; using std::transform; using std::tolower;
    transform(begin(str), end(str), begin(result), tolower);
    return result;
}

Client code:

std::string a = "abcDe";
std::wstring b = L"abcdeF";

auto lower_a = toLower(a);
auto lower_b = toLower(b);
\$\endgroup\$
  • \$\begingroup\$ The problem with using templates is that it doesn't help the code choose to use the correct lowercasing function. For wide chars you need std::towlower as @Loki said. \$\endgroup\$ – Snowbody Apr 15 '15 at 19:03
  • \$\begingroup\$ @Snowbody, I am using std::tolower for both wide chars and narrow chars. I am not sure what you are trying to say. \$\endgroup\$ – utnapistim Apr 16 '15 at 8:44
  • \$\begingroup\$ check the docs for std::tolower(). Even though its parameter is an int and it returns an int it is specified to only accept char values, plus EOF. It does not work properly for Unicode characters. \$\endgroup\$ – Snowbody Apr 16 '15 at 12:48

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.