CODEWITHSUNDEEP
c++stringfilememory-management
NathanielJPerkins
NathanielJPerkins

Reputation: 273

CodeEval Challenge: Reverse strings in input file

I've decided to start learning C++ before I take a formal class on it next year, I've begun with some of the easy challenges on CodeEval and Project Euler. In this one, you have to take an input file which has strings of words, and you have to output the line of the file with the words reversed. Such that a file with the following input

1: This is line one

2: This is line two

would end up as

1: one line is This

2: two line is This

I wrote the following program to do that, and aside from not properly reversing the string, instead fully reversing the word, it Segmentation faults despite compiling with no errors or warnings. I assume I've missed something about proper memory management in C++ but I'm not sure what it is. So can someone enlighten me on what I've missed in this regarding memory management?

#include <iostream>
#include <fstream>
#include <string>
#include <vector>
#include <sstream>
int main(int argc, char** argv)
{
  std::string filename = argv[1]; //has to be argv[1], argv[0] is program name
  std::string output_string; //final output
  std::string line; //Current line of file
  std::ifstream read(filename.c_str());
  if(read.is_open()){
    while(std::getline(read,line)){
      std::string temp;
      std::istringstream iss;
      iss.str(line);
      while(iss >> temp){ //iterates over every word
        output_string.insert(0,temp); //insert at the start to reverse
        output_string.insert(0," "); //insert spaces between new words
      }
      output_string.erase(0,1); //Removes the space at the beginning
      output_string.insert(0,"\n"); //Next line
    }
    output_string.erase(0,1); //Remove final unnecessary \n character
    read.close();
  }
  else{
    std::cout<<"Unable to open file\n";
  }
  for(unsigned int i = output_string.length(); i>=0;i--){
    std::cout<<output_string[i];
  }
  std::cout<<"\n";
}

Upvotes: 1

Views: 81

Answers (2)

notmyfriend
notmyfriend

Reputation: 245

for(unsigned int i = output_string.length(); i>=0;i--){
   std::cout<<output_string[i];
}

The segfault happens here; you might be able to get a warning from the compiler with some additional flags. e.g. g++ produces no warnings with -Wall, but produces two warnings with -Wextra: one about argc not being used, and the other about this loop never terminating.

The issue here is twofold: as Captain Giraffe said, you're starting beyond the actual length your strings; but also the condition i >= 0 will always be true, because i is unsigned. Therefore once it reaches 0, the next decrement will cause it to wrap around to the highest possible value, and then you definitely get an out-of-bounds memory access.

The warning reported is:

reverse.cpp:31:49: warning: comparison of unsigned expression >= 0 is always true [-Wtype-limits]
   for(unsigned int i = output_string.length(); i>=0;i--){

Also as Captain Giraffe said, you're reversing the entire file, not just each line. You can therefore just reverse each line and output it once you've finished the line, rather than storing the entire output for later.

Here's the entire program with minimal changes to avoid any warnings and get the correct output. The main change is moving all usage of output_string into the read loop.

int main(int argc, char** argv)
{
    if (argc != 2)
    {
            std::cerr << "Need a file to process!" << std::endl;
            return 1;
    }
    std::string filename = argv[1]; //has to be argv[1], argv[0] is program name
    std::string line; //Current line of file
    std::ifstream read(filename.c_str());
    if(read.is_open()){
            while(std::getline(read,line)){
                    std::string output_string; //final output
                    std::string temp;
                    std::istringstream iss;
                    iss.str(line);
                    while(iss >> temp){ //iterates over every word
                            output_string.insert(0,temp); //insert at the start to reverse
                            output_string.insert(0," "); //insert spaces between new words
                    }
                    output_string.erase(0,1); //Removes the space at the beginning
                    std::cout << output_string << std::endl;
            }
            read.close();
    }
    else{
            std::cout<<"Unable to open file\n";
    }
}

Upvotes: 1

Captain Giraffe
Captain Giraffe

Reputation: 14705

Change the last for-statement to 

std::cout << output_string;

You are starting off the output by printing the character after the last in the output string. This gets rid of the segfault. Now you are trying to print the reversed output in reverse.

Now you find that you should just reverse each line, not the entire text. You can easily do that by adding a starting index for each line, instead of 0, in your inserts.

So instead of 

output_string.insert(0,temp); //insert at the start to reverse

You can do

output_string.insert(start_of_line, temp); //insert at the start to reverse

Upvotes: 1

Related Questions