C++ String Non-Member Operator Overloading

Question

This is a practice for operator overloading, I am well aware that some of these don't make sense. I am seeking review only on the code/practices, not the purpose.

/* (C) 2017 ZACH HILMAN
 * PROPRIETARY/CONFIDENTIAL. ALL RIGHTS RESERVED
 *
 *
 *
 *
 *
 */
#ifndef COMPUTERSCIENCE_ENHANCED_STRING_H
#define COMPUTERSCIENCE_ENHANCED_STRING_H

#include <string>

/// Remove the first occrance of rhs from lhs.
inline std::string operator-(const std::string& lhs, const std::string& rhs) {
    auto loc = lhs.find(rhs);
    if (loc != -1) return lhs.substr(0, loc) + lhs.substr(loc + rhs.size());
    return std::string();
}

inline std::string& operator-=(std::string& lhs, const std::string& rhs) {
    return lhs = operator-(lhs, rhs);
}

/// Repeat lhs rhs times.
inline std::string operator*(const std::string& lhs, unsigned long rhs) {
    std::string out;
    while (rhs --> 0) out += lhs;
    return out;
}

inline std::string& operator*=(std::string& lhs, unsigned long rhs) {
    return lhs = operator*(lhs, rhs);
}

/// Find the number of times rhs appears in lhs.
inline unsigned long operator/(const std::string& lhs, const std::string& rhs) {
    if (rhs == "") return static_cast<unsigned long>(-1);
    unsigned long out = 0;
    for (unsigned long s_loc = 0, a; s_loc < lhs.size();) {
        if ((a = lhs.find(rhs, s_loc)) != -1) ++out, s_loc = a + 1;
        else break;
    }
    return out;
}

/// Remove all occurances of rhs from lhs.
inline std::string operator%(const std::string& lhs, const std::string& rhs) {
    if (rhs == "") return lhs;
    auto out = lhs;
    std::string::size_type pos;
    while ((pos = out.find(rhs)) != std::string::npos) out = out.replace(pos, rhs.size(), "");
    return out;
}

inline std::string& operator%=(std::string& lhs, const std::string& rhs) {
    return lhs = operator%(lhs, rhs);
}

// Subtract str from STRING_CHARSET (Not done-- will eventually be all ASCII characters).
inline std::string operator~(const std::string& str) {
    static const std::string STRING_CHARSET = "";
    return operator-(STRING_CHARSET, str);
}

// Find the intersection of characters of lhs and rhs.
inline std::string operator&(const std::string& lhs, const std::string& rhs) {
    std::string out;
    for (auto c : lhs) if (rhs.find(c) != std::string::npos) out += c;
    return out;
}

inline std::string& operator&=(std::string& lhs, const std::string& rhs) {
    return lhs = operator&(lhs, rhs);
}

/// Find the union of characters of lhs and rhs.
inline std::string operator|(const std::string& lhs, const std::string& rhs) {
    std::string out;
    for (auto c : lhs) if (out.find(c) == std::string::npos) out += c;
    for (auto r : rhs) if (out.find(r) == std::string::npos) out += r;
    return out;
}

inline std::string& operator|=(std::string& lhs, const std::string& rhs) {
    return lhs = operator|(lhs, rhs);
}

/// Find the XOR of characters of lhs and rhs.
inline std::string operator^(const std::string& lhs, const std::string& rhs) {
    std::string out;
    for (auto c : lhs) if (rhs.find(c) == std::string::npos) out += c;
    for (auto r : rhs) if (lhs.find(r) == std::string::npos) out += r;
    return out;
}

inline std::string& operator^=(std::string& lhs, const std::string& rhs) {
    return lhs = operator^(lhs, rhs);
}

/// Add rhs spaces to the front of lhs.
inline std::string operator>>(const std::string& lhs, unsigned long rhs) {
    return std::string(rhs, ' ') + lhs;
}

inline std::string& operator>>=(std::string& lhs, unsigned long rhs) {
    return lhs = operator>>(lhs, rhs);
}

/// Add rhs spaces to the end of lhs.
inline std::string operator<<(const std::string& lhs, unsigned long rhs) {
    return lhs + std::string(rhs, ' ');
}

inline std::string& operator<<=(std::string& lhs, unsigned long rhs) {
    return lhs = operator<<(lhs, rhs);
}


#endif //COMPUTERSCIENCE_ENHANCED_STRING_H

Ben Steffan · Accepted Answer · 2017-10-12 20:53:52Z

Style Advice

Do not try to do too much on one line. Constructs such as if ((a = lhs.find(rhs, s_loc)) != -1) ++out, s_loc = a + 1; are difficult to read, difficult to parse and prone to errors. Instead, let each control statement have its own block, and do not abuse the comma operator to do completely unrelated things. For example, the line I mentioned should rather be something like
```
if ((a = lhs.find(rhs, s_loc)) != -1) {
    ++out;
    s_loc = a + 1;
}
```
or, even better,
```
a = lhs.find(rhs, s_loc);
if (a != -1) {
    ++out;
    s_loc = a + 1;
}
```

Avoid magic numbers. In particular, when checking the result of string::find, check whether the result is equal to std::string::npos, not -1. There even are some places in the code where you are already doing this, so please adopt it for the remaining places as well.
Prefer early return on failure. Concretely, in operator- you return an empty string only after you have checked for and possibly returned a valid result. Although this is quite minor, I would have expected the method to work the other way around, i. e.
```
if (loc == std::string::npos) {
    return std::string();
}
return lhs.substr(0, loc) + lhs.substr(loc + rhs.size());
```
which is quite a common pattern.
if (rhs == "") return static_cast<unsigned long>(-1); works but is not expressive. Prefer
```
if (rhs.empty()) {
    return std::numeric_limits<unsigned long>::max();
}
```
while (rhs --> 0) out += lhs; does nothing but obscure what is going on. --> is not an operator in C++, so you should not write it as if it were one. Write while (rhs-- > 0) ...
Remove the typo from the comment to operator-; what currently reads "occrance" should probably be "occurance".

Possible Optimizations

Although generally your code appears to work well, there are some things that you could easily implement to improve performance, mostly to avoid unnecessary copies.

While it is good practice to implement operators in terms of other operators, the way that you implemented your operation + assignment-operators incurs making additional copies of your input. For example, let's consider operator% and operator%=:

inline std::string operator%(const std::string& lhs, const std::string& rhs) {
    //...
    auto out = lhs;
    //...
    return out;
}

inline std::string& operator%=(std::string& lhs, const std::string& rhs) {

    return lhs = operator%(lhs, rhs);
}

Since operator% does not modify its inputs, it has to make a copy which is then modified and returned. However, operator%= does modify one of its inputs, in this case lhs, so making a copy, modifying it and reassigning it to lhs is unnecessary. Instead, you could approach the problem the other way around: First, implement operator%=, then implement operator% in terms of it:

inline std::string& operator%=(std::string& lhs, const std::string& rhs) {
    //perform in-place operation on lhs
    return lhs;
}

inline std::string operator%(const std::string& lhs, const std::string& rhs) {
    std::string out = lhs;
    out %= rhs;
    return out;
}

Thanks for the help! So would you suggest swapping the operator dependency on all or just the ones where I copy? (Also wrote the doxygen at 12am just so I could describe how they work) — Zach Hilman
– Zach Hilman, Commented Oct 13, 2017 at 0:09
@ZachHilman For consistency, I would say yes although it isn't strictly necessary. It's just nice to have structure in your code, even when it's only "This group of operators is implemented in terms of another group" . — Ben Steffan
– Ben Steffan, Commented Oct 13, 2017 at 5:10

Snowhawk · Accepted Answer · 2017-10-13 00:32:13Z

Not everyone may want these unconventional operators by default. Don't pollute the global namespace. Group these operators into a named namespace and let users opt-in when they want to use them.

The inplace operations do too much work. Consider implementing the copy-dependent operations in terms of in-place operations.

Consider providing named functions for those that don't want to use overloaded operators. Have the overloaded operators call these named functions.

Consider using a string view for read-only arguments to eliminate allocations from implicit conversions.

You reuse the same variable names (i.e. out, rhs). Use names to reflect how it's being used. For out, you could use union_set, difference_set, intersected, etc. For rhs, the unsigned long arguments are better described as count.

For code not implemented, you should make them uncallable (remove/comment) or statically assert that it's not been implemented.

/// Remove the first occurrence of rhs from lhs.
inline std::string operator-(const std::string& lhs, const std::string& rhs) {
    auto loc = lhs.find(rhs);
    if (loc != -1) return lhs.substr(0, loc) + lhs.substr(loc + rhs.size());
    return std::string();
}

If rhs is not found in lhs, shouldn't you just return lhs?

If you do remove the first occurrence, you could have up to 3 possible allocations here. Consider reserving a buffer first then copy the two substrings.

inline std::string operator*(const std::string& lhs, unsigned long rhs) {
    std::string out;
    while (rhs --> 0) out += lhs;
    return out;
}

Depending on the length of lhs and the value of rhs, this could result in more than 1 allocation. Consider reserving first.

Careful with your spacing (rhs --> 0 vs rhs-- > 0). Since you are just looping until rhs is 0, your conditional could simply be rhs--.

inline unsigned long operator/(const std::string& lhs, const std::string& rhs) {
    if (rhs == "") return static_cast<unsigned long>(-1);
    unsigned long out = 0;
    for (unsigned long s_loc = 0, a; s_loc < lhs.size();) {
        if ((a = lhs.find(rhs, s_loc)) != -1) ++out, s_loc = a + 1;
        else break;
    }
    return out;
}

Prefer using std::size_t as it is defined to be able to represent the size of any object in bytes. The max value of unsigned long depends on the particular system and library implementation. ULONG_MAX can be \$2^{32} - 1\$ or greater.

You can simplify the loop.

std::size_t count = 0;
for (auto s_loc = lhs.find(rhs, 0); s_loc != std::string::npos;
     s_loc = lhs.find(rhs, s_loc+1)) {
    ++count;
}

inline std::string operator%(const std::string& lhs, const std::string& rhs) {
    if (rhs == "") return lhs;
    auto out = lhs;
    std::string::size_type pos;
    while ((pos = out.find(rhs)) != std::string::npos) out = out.replace(pos, rhs.size(), "");
    return out;
}

Consider the overlapping occurrence case of searching ababa for the substring aba. Using your count operator, a count of \$2\$ is returned. If I were to use this operator to erase all occurrences, only one occurrence would be erased.

If I put them all into a namespace, can I remove the inline? I know inline is a hint to the compiler, but I do not understand why it is required in this instance. — Zach Hilman
– Zach Hilman, Commented Oct 13, 2017 at 1:16
Namespacing these functions won't effect inlining. inline remains a hint to the compiler and the compiler is free to accept or ignore the hint. — Snowhawk
– Snowhawk, Commented Oct 13, 2017 at 2:44

Stack Exchange Network

C++ String Non-Member Operator Overloading

2 Answers 2

Style Advice

Possible Optimizations

You must log in to answer this question.

Hot Network Questions

C++ String Non-Member Operator Overloading

2 Answers 2

Style Advice

Possible Optimizations

You must log in to answer this question.

Related

Hot Network Questions