Segmentation fault when trying to sort strings from a text file.

[kada17]

I am trying to make a function that takes strings from a file as an argument and sorts them using another function. What happens is that the function looks at the first letter of the string in the first and second line and starts an insertion sort. It calls on another function to help it determine which of the letters from the two strings come first. At the end if the file originally contained

abc
xda
wol
tom
colonel

The result would be

abc
colonel
tom
wol
xda

My code:

void sort(char* *begin, char* *endchar)
{
  if (orderbool(*begchar, *endchar) == true )
  {
  char * i;
  char * j;
  char * k;
  char   t;
  for (i = *begchar; i != *endchar; ++i)
  {
    t = *i;
    for (k = i, j = k--; j != *begchar && t < *k; --j, --k)
      *j = *k;
    *j = t;
  }
  }
}

This is the template we are supposed to use for the assignment. It compiles without any errors but when I run it, it gives a segmentation fault error. I suspect it may be something to do with the pointers. Would like some help!

[CPPAWhile]

Just a comment or two so far to get started (can you please show your piece of code that calls this function? so we can see how you called it):

1) the method sort has two parameters, begchar, and endchar, each of which is a double pointer of type char (char **). This means that you must call it using the address of a pointer. For example: The calling function might have a char buffer, which has a pbuf = &buffer, in which case you would pass &pbuf, not just pbuf. A double pointer is normally used either 1) so you can change the pointed to "object", or it's used for an array of strings. Try to be clear what you want here - do you want to pass an array of strings for each or do you want to change the char * (I assume the former).

And be careful, you say char* *begin in the first line (I believe you meant to say char* *begchar).

I can see that you want to pass the beginning and ends of an array that contains strings as each element, that is obvious. And I can see that you want to change the order of them in a sort. Also, keep in mind that if you change the order, that each string must have enough space for a potential new string which is larger. So you must use a sort of "max-size" for each element of the passed in array. Normally, strings have a '/0' terminator. I just want to be sure that your strings are terminated, so you can tell when the next one occurs. I'm assuming you're not stuffing them in, if you did, you would never know when the next string was.

I need to look at this some more, but don't you need to call orderbool for each string, not just once in the function?

It's a fancy method so it will take me a while - it's confusing when I see a bunch of pointers and I don't see an obvious connection between the pointers and the buffers themselves, sorry. Add a couple printf statements, it might help you see what's stored in the variable that's causing the segmentation fault. So printf("some variable = %s\n", var);

[Ganado]

That's quite a mess. It doesn't compile for me because you're using deprecated methods of using char*'s instead of const char*'s.

Because I'm not exactly sure what you're doing in there (but it looks like you've at least tried), I'm just going to re-write it in a way I understand, and if you like it, you have it.

// Example program
#include <iostream>
#include <cstring>

void sort(const char* *begchar, const char* *endchar)
{
  const char ** i;
  const char ** j;

  for (i = begchar; i != endchar; i++)
  {
      for (j = i; j != endchar; j++)
      {
        if (strcmp(*j, *i) < 0)
        {
          // swap
          const char* temp = *i;
          *i = *j;
          *j = temp;
        }
      }
  }
}

int main()
{
  const int size = 4;
  const char* stuff[size] = {"apple", "cherry", "yam", "banana",};

  sort(stuff, stuff + size); // O(n^2) sort (there are more efficient options available,
                      // but they are a bit more complicated to implement, see: mergesort)
  
  for (int i = 0; i < size; i++)
    std::cout << stuff[i] << std::endl;
}

apple
banana
cherry
yam

Edit: Well, you could remove the consts if you have it point to a non-string literal instead.

// Example program
#include <iostream>
#include <cstring>

using str_type = char*;
void sort(str_type *begchar, str_type *endchar)
{
  str_type* i;
  str_type* j;

  for (i = begchar; i != endchar; i++)
  {
      for (j = i; j != endchar; j++)
      {
        if (strcmp(*j, *i) < 0)
        {
          // swap
          str_type temp = *i;
          *i = *j;
          *j = temp;
        }
      }
  }
}

int main()
{
  char word1[256] = "apple";
  char word2[256] = "cherry";
  char word3[256] = "yam";
  char word4[256] = "banana";
    
  const int size = 4;
  char* stuff[size];
  stuff[0] = word1;
  stuff[1] = word2;
  stuff[2] = word3;
  stuff[3] = word4;
  
  sort(stuff, stuff + size);
  
  for (int i = 0; i < size; i++)
    std::cout << stuff[i] << std::endl;
}

But basically, in modern C++, you can't do
char* a = "apple";
because "apple" is static data in the executable so you shouldn't be able to write to it.
so you use const instead, const char* a = "apple";

But if you copy the string literal into an array, and then use the array, it need not be const.

[Thomas1965]

Do you have to do it in C ?
Much, much easier in C++ with the string and vector class.

[kada17]

Actually, it was a silly mistake on my part. I have fixed the problem now. Appreciate all of you guys' time!