How to create a function that removes all of a selected character in a C-string?

I want to make a function that removes all the characters of ch in a c-string. But I keep getting an access violation error.

Unhandled exception at 0x000f17ba in testassignments.exe: 0xC0000005: Access violation writing location 0x000f787e.

void removeAll(char* &s, const char ch)
{
    int len=strlen(s);
    int i,j;
    for(i = 0; i < len; i++)
    {
        if(s[i] == ch)
        {
            for(j = i; j < len; j++)
            {
                s[j] = s[j + 1];
            }
            len--;
            i--;    
        } 
    }
    return;
}

I expected the c-string to not contain the character "ch", but instead, I get an access violation error. In the debug I got the error on the line:

s[j] = s[j + 1];

I tried to modify the function but I keep getting this error.

Edit-- Sample inputs:

s="abmas$sachus#settes";
ch='e' Output->abmas$sachus#settes, becomes abmas$sachus#stts
ch='t' Output-> abmas$sachus#stts, becomes abmas$sachus#ss.

Instead of producing those outputs, I get the access violation error. Edit 2: If its any help, I am using Microsoft Visual C++ 2010 Express.

Apart from the inefficiency of your function shifting the entire remainder of the string whenever encountering a single character to remove, there's actually not much wrong with it.

In the comments, people have assumed that you are reading off the end of the string with s[j+1], but that is untrue. They are forgetting that s[len] is completely valid because that is the string's null-terminator character.

So I'm using my crystal ball now, and I believe that the error is because you're actually running this on a string literal.

// This is NOT okay!
char* str = "abmas$sachus#settes";
removeAll(str, 'e');

This code above is (sort of) not legal. The string literal "abmas$sachus#settes" should not be stored as a non-const char*. But for backward compatibility with C where this is allowed (provided you don't attempt to modify the string) this is generally issued as a compiler warning instead of an error.

However, you are really not allowed to modify the string. And your program is crashing the moment you try.

If you were to use the correct approach with a char array (which you can modify), then you have a different problem:

// This will result in a compiler error
char str[] = "abmas$sachus#settes";
removeAll(str, 'e');

Results in

error: invalid initialization of non-const reference of type ‘char*&’ from an rvalue of type ‘char*’

So why is that? Well, your function takes a char*& type that forces the caller to use pointers. It's making a contract that states "I can modify your pointer if I want to", even if it never does.

There are two ways you can fix that error:

1. The TERRIBLE PLEASE DON'T DO THIS way:

   // This compiles and works but it's not cool!
   char str[] = "abmas$sachus#settes";
   char *pstr = str;
   removeAll(pstr, 'e');
   

   The reason I say this is bad is because it sets a dangerous precedent. If the function actually did modify the pointer in a future "optimization", then you might break some code without realizing it.

   Imagine that you want to output the string with characters removed later, but the first character was removed and you function decided to modify the pointer to start at the second character instead. Now if you output str, you'll get a different result from using pstr.

   And this example is only assuming that you're storing the string in an array. Imagine if you actually allocated a pointer like this:

   char *str = new char[strlen("abmas$sachus#settes") + 1];
   strcpy(str, "abmas$sachus#settes");
   removeAll(str, 'e');
   

   Then if removeAll changes the pointer, you're going to have a BAD time when you later clean up this memory with:

   delete[] str;  //<-- BOOM!!!
   

2. The I ACKNOWLEDGE MY FUNCTION DEFINITION IS BROKEN way:

   Real simply, your function definition should take a pointer, not a pointer reference:

   void removeAll(char* s, const char ch)
   

   This means you can call it on any modifiable block of memory, including an array. And you can be comforted by the fact that the caller's pointer will never be modified.

   Now, the following will work:

   // This is now 100% legit!
   char str[] = "abmas$sachus#settes";
   removeAll(str, 'e');
   

---

Now that my free crystal-ball reading is complete, and your problem has gone away, let's address the elephant in the room:

Your code is needlessly inefficient!

1. You do not need to do the first pass over the string (with strlen) to calculate its length

2. The inner loop effectively gives your algorithm a worst-case time complexity of O(N^2).

3. The little tricks modifying len and, worse than that, the loop variable i make your code more complex to read.

What if you could avoid all of these undesirable things!? Well, you can!

Think about what you're doing when removing characters. Essentially, the moment you have removed one character, then you need to start shuffling future characters to the left. But you do not need to shuffle one at a time. If, after some more characters you encounter a second character to remove, then you simply shunt future characters further to the left.

What I'm trying to say is that each character only needs to move once at most.

There is already an answer demonstrating this using pointers, but it comes with no explanation and you are also a beginner, so let's use indices because you understand those.

The first thing to do is get rid of strlen. Remember, your string is null-terminated. All strlen does is search through characters until it finds the null byte (otherwise known as 0 or '\0')...

[Note that real implementations of strlen are super smart (i.e. much more efficient than searching single characters at a time)... but of course, no call to strlen is faster]

All you need is your loop to look for the NULL terminator, like this:

for(i = 0; s[i] != '\0'; i++)

Okay, and now to ditch the inner loop, you just need to know where to stick each new character. How about just keeping a variable new_size in which you are going to count up how long the final string is.

void removeAll(char* s, char ch)
{
    int new_size = 0;
    for(int i = 0; s[i] != '\0'; i++)
    {
        if(s[i] != ch)
        {
            s[new_size] = s[i];
            new_size++;
        } 
    }

    // You must also null-terminate the string
    s[new_size] = '\0';
}

If you look at this for a while, you may notice that it might do pointless "copies". That is, if i == new_size there is no point in copying characters. So, you can add that test if you want. I will say that it's likely to make little performance difference, and potentially reduce performance because of additional branching.

But I'll leave that as an exercise. And if you want to dream about really fast code and just how crazy it gets, then go and look at the source code for strlen in glibc. Prepare to have your mind blown.