6

I'd like to start by saying that yes, I Google'd this problem before coming here, and none of the answers seemed relevant.

I stole the below code from What's the best way to trim std::string?, because for whatever reason, there is no standard c++ trim function. Using Visual Studio, it compiled fine, and I managed to complete the rest of the project without it giving me any errors.

This morning though, I decided to try compiling the entire project manually (using g++ -std=c++11 *.cpp), and now suddenly the trim functions are yielding the following error:

DVD.cpp: In static member function 'static DVD DVD::parseDVD(std::string, std::string)':
DVD.cpp:65:59: error: invalid initialization of non-const reference of type 'std::string& {aka std::basic_string<char>&}
 from an rvalue of type 'std::basic_string<char>'
 std::string rID = trim(dataStr.substr(0, preTitlePos - 1));

It yields similar errors for the other 2 times that trim is used.

Here is the "stolen" code:

(Utils.h):

static inline std::string& ltrim(std::string& s) {
    s.erase(s.begin(), std::find_if(s.begin(), s.end(), std::not1(std::ptr_fun<int, int>(std::isspace))));
    return s;
}

// trim from end
static inline std::string& rtrim(std::string& s) {
    s.erase(std::find_if(s.rbegin(), s.rend(), std::not1(std::ptr_fun<int, int>(std::isspace))).base(), s.end());
    return s;
}

// trim from both ends
static inline std::string& trim(std::string& s) {
    return ltrim(rtrim(s));
}

And here is the parseDVD function that the error mentions:

(DVD.cpp):

DVD DVD::parseDVD(std::string dataStr, std::string delimiter) {
    DVD newDVD;
    int preTitlePos = dataStr.find(delimiter, 0);
    int preGenrePos = dataStr.find(delimiter, preTitlePos + 1);

    // V Error is here V
    std::string rID = trim(dataStr.substr(0, preTitlePos - 1));
    std::string rTitle = trim(dataStr.substr(preTitlePos + 1, preGenrePos - preTitlePos - 1));
    std::string rGenre = trim(dataStr.substr(preGenrePos + 1));

    int parsedID = 0;
    //Requirements for a successful parse
    //The ID must be fully numeric, and both of the delimiters must have been found
    if (parseInt(rID, parsedID) && preTitlePos > -1 && preGenrePos > -1) {
        return
            newDVD  .setID(parsedID)
                    .setTitle(rTitle)
                    .setGenre(rGenre);
    }

    return badDVD;
}

If I remove all of the &s from the trim functions, it works, but I'd rather it not make copies constantly.

This baffles me because I know the code is sound; not only is it the accepted answer to the above question, but it works fine in Visual Studio.

Community
  • 1
  • 1
Carcigenicate
  • 43,494
  • 9
  • 68
  • 117
  • 1
    Assuming they are free functions, it's unusual to declare a function in a header file both static and inline. I believe it may hinder the linker's efforts to reduce the executable size, but I'm not sure. – Neil Kirk Mar 23 '15 at 13:04
  • @Neil Kirk I copied his code, but he probably didn't intend for it to be put directly in a header. I'll get rid of inlining; especially since they mention in our textbook that explicit inlining is frowned upon. Thanks. – Carcigenicate Mar 23 '15 at 15:49
  • 1
    If you want a function in a header file, inlining is generally preferred to making them static, unless you have a special reason not to. Don't believe everything you read in a textbook. – Neil Kirk Mar 23 '15 at 15:49
  • Is there a reason it's preferred? Performance? – Carcigenicate Mar 23 '15 at 15:51
  • 1
    Functions in a header file are compiled again for every source file they are included in, which is slower but inevitable, whether static or inline. When the function is inline and not static, the linker picks only one version to make it in the final exe (as they are all the same). However, making them static tells the linker to consider them as independent functions, even though they end up being the same. So I'm not sure if linker will optimize out the extra copies in that case. – Neil Kirk Mar 23 '15 at 16:13

1 Answers1

7

Your trim() function is expecting a non-const reference to a std::string. When you invoke it like this:

std::string rID = trim(dataStr.substr(0, preTitlePos - 1));

You are invoking it with an rvalue, an unnamed temporary. This will have type const std::string &, so it is not compatible with the trim() function. In order to make it work, you would need to assign it to a named variable first:

std::string temp = dataStr.substr(0, preTitlePos - 1);
std::string rID = trim(temp);

On a side note, you seem to be mixing two methods of returning outputs from the trim() function. Typically, one will either return the function's result (usually by value), or one will modify arguments that were passed to the function by reference. Your functions do both, which is not typical.

I would change them to either take an input argument and return the result by value, or modify the by-reference argument that you pass in (and change their return types to void).

Jason R
  • 11,159
  • 6
  • 50
  • 81
  • Why does VS allow this? – Carcigenicate Mar 23 '15 at 12:53
  • And so, to get this to work, I need to assign the `dataStr...` part to a named variable, and then pass it to `trim`? – Carcigenicate Mar 23 '15 at 12:55
  • And as noted above, I actually stole the trim functions from the above link; and they were the accepted answer there (although I didn't go through the comments below). He's probably having it pass by reference to avoid copying? – Carcigenicate Mar 23 '15 at 12:57
  • 1
    Maybe that was the intent, but it likely won't be effective. Since you're using C++11, I would return by value whenever needed. C++11 compilers can be more selective about eliding copies when appropriate. – Jason R Mar 23 '15 at 13:00
  • Thank you. Do you have any idea why this was never a problem in VS though? That's what's causing me the most confusion. You'd think if it's an error (an not just a warning) that it should be caught; regardless of the environment. I downloaded VS only a month ago (around the same time I installed MinGW), so it should be the latest version. – Carcigenicate Mar 23 '15 at 15:53
  • 1
    @Carcigenicate: Visual Studio is non-standards-conformant in many areas, even in the latest versions. gcc and clang are much more strict adherents to the language standards, so I would use the combination of those as a better measuring stick on what's right and wrong. They aren't perfect, but they are much more so than MSVC. – Jason R Mar 23 '15 at 16:08
  • Cool, thank you. I'll make sure to manually compile my sources before I consider them OK. – Carcigenicate Mar 23 '15 at 17:15