Segmentation fault when remove duplicate value in pointer array

Question

When debugging, I found that segmentation fault caused after copy from temp array (b) to original array (a), I don't know how to solve this, can you guys help me on that?

#include <iostream>
using namespace std;

int main() {
    int n;
    cout << "n = ";
    cin >> n;
    int *a = new int[n];
    
    for (int i = 0; i < n; i++)
        cin >> a[i];

    cout << "\nNumber to remove: ";
    int k;
    cin >> k;

    for (int i = 0; i < n; i++)
    {
        while (a[i] == k && i < n - 1)
        {
            int *b = new int[n]{0};
            for (int j = 0; j < i; j++)
                b[j] = a[j];
            for (int j = i + 1; j < n; j++)
                b[j - 1] = a[j];

            a = NULL;
            delete a;
            n--;
            int *a = new int[n];
            for (int j = 0; j < n; j++)
                a[j] = b[j];
        }
    }

    cout << "Result: ";
    for (int i = 0; i < n; i++)
        cout << a[i] << " ";
}

`int *a = new int[n];` inside the loop must be `a = ...`. You are declaring a _new_ `a`, that is unelated to the `a` from outside the loop. (The first `a`, from outside the loop, will be `NULL` at the end of the loop and never change again. — Lukas-T, Nov 16 '21 at 12:12
@Chí Bằng Hoàng The reason of the error is the awful and too complicated code. To remove a value from an array there is no any need to allocate dynamically an auxiliary array each time then the target value is removed. — Vlad from Moscow, Nov 16 '21 at 12:20
@MSalters It works very well. The problem is that there is a memory leak.:) — Vlad from Moscow, Nov 16 '21 at 12:23
Well, I learned that I need to copy original array when delete it, may be I'm wrong. So what is the better solution? — Kritiqual, Nov 16 '21 at 12:48
@ChíBằngHoàng *what is the better solution?* -- `auto iter = std::remove(a, a + n, k);` and then `iter` points to the end of the valid values in the array. You don't need all of this convoluted code to remove a value from an array. — PaulMcKenzie, Nov 16 '21 at 13:41
@PaulMcKenzie thanks for your suggest, but I haven't learn that yet — Kritiqual, Nov 16 '21 at 14:08
@ChíBằngHoàng: If you're doing memory management yourself, then yes, you should delete the original array `a`. The problem is that you first set `a=NULL` so the next line is effectively `delete NULL`. You don't delete the `new int[n]`. BTW, if you really, really must use `new[]`, then it should also be `delete[]`. It only "works" now because `delete NULL` and `delete[] NULL` both do nothing. But just use `vector`, really. — MSalters, Nov 16 '21 at 14:12
My advice (if you can't use a std::vector) is don't reallocate a smaller array each time. You are doing way too much allocation. You also should remove the `b` dynamic array completly. — drescherjm, Nov 16 '21 at 14:19
@ChíBằngHoàng *but I haven't learn that yet* -- And most people asking a question on stackoverflow didn't learn yet what they are given as an answer or as a comment. Wouldn't now be the best time to learn? Every other profession works this way, where you are being taught new and better things, so why not programming? — PaulMcKenzie, Nov 16 '21 at 14:24
This question should help you remove an item from an array without needing to allocate a second array: [https://stackoverflow.com/questions/879603/remove-an-array-element-and-shift-the-remaining-ones](https://stackoverflow.com/questions/879603/remove-an-array-element-and-shift-the-remaining-ones) — drescherjm, Nov 16 '21 at 14:49
Thank y'all for the answer, I will take a look into them :|| — Kritiqual, Nov 16 '21 at 15:18
Please clarify your specific problem or provide additional details to highlight exactly what you need. As it's currently written, it's hard to tell exactly what you're asking. — Community, Nov 18 '21 at 10:53

Roberto Montalti · Answer 1 · 2021-11-16T13:03:54.177

The problem with this implementation lies here:

a = NULL;
delete a;

When you use the delete keyword you are telling the compiler to free the memory allocated by a new keyword, in this case you have a which is a pointer to the memory allocated by int *a = new int[n];. a at this point would probably be a 32bit address for example 0xbfebd5c0, i said probably because it depends on the architecture of the machine, however... after that you are setting a to NULL resulting in a being 0x0, at the next while condition check a is NULL and accessing 0x0 + i is not possible due to safety reasons imposed by the OS (no one gave you access to this location). The solution to your specific problem would be removing a = NULL; and delete a; as both these operations interfere with the while condition check.

I would suggest to take another approach at solving this problem since you really don't need to do this many dynamic allocations.

While the answer to remove `a = NULL;` is correct, the explanation isn't. Deleting a null-pointer is perfectly fine. But you are correct that deleting an arbitray address would cause an error. Null-pointers are special in that regard. — Lukas-T, Nov 16 '21 at 12:39
Oh yeah, you are totally right, the actual reason is that he is looping on a condition which depends on `a` hence accessing some displaced location in respect to 0x0, which he isn't allowed to do. Thanks @churill — Roberto Montalti, Nov 16 '21 at 12:52

Segmentation fault when remove duplicate value in pointer array

1 Answers1