What is the exact behaviour of delete and delete[]?

Question

Why is this code wrong? Am I missing something regarding the behaviour of delete and delete[]?

void remove_stopwords(char** strings, int* length) 
{
    char** strings_new = new char*[*length];
    int length_new = 0;

    for(int i=0; i<*length; i++) {
        if(is_common_keyword(strings[i]) == 0) {
            strings_new[length_new] = strings[i];
            length_new++;
        }
        else {
            delete strings[i];
            strings[i] = nullptr;
        }
    }
    delete[] strings;

    strings = new char*[length_new];
    for(int i=0; i<length_new; i++) {
        strings[i] = strings_new[i];
    }
    delete[] strings_new;
    *length = length_new;
}

Explanations: this code should take an array of C-style strings and remove some particular strings of them; the array of C-style strings was created using new[] and every C-style string was created using new. The result of the code is that no word is canceled, but the array is only sliced.

for `delete` vs `delete[]`: https://stackoverflow.com/questions/2425728/delete-vs-delete-operators-in-c — Assem, May 22 '15 at 20:42
This is the most incomprehensible bit of code I have seen for a long time — Ed Heal, May 22 '15 at 20:42
You re-assign to your by-value parameter `strings`. I hope you are not expecting the newly assigned value to be visible outside of your function? — , May 22 '15 at 20:43
a. What do you mean no word is cancelled? b. what is the value of length_new in the end, and what is the value of length at the start? — Amit, May 22 '15 at 20:43
I can't read this, too many pointer stars there, but you probably want to have `char* &strings` instead of `char** strings`. — Marius Bancila, May 22 '15 at 20:46
Is it me or should code have a narrative. This code does not — Ed Heal, May 22 '15 at 20:51
Per your title, the *exact* behavior is implementation defined. Some can mark the items as not in use, others can zero them out. — Thomas Matthews, May 22 '15 at 20:53

score 6 · Accepted Answer · answered May 22 '15 at 20:53

I don't see any problem in the use of new[] or delete[] in the code shown.

No, wait.

I see a lot¹ of problems, but your intent is clear and the code seems doing what you want it to do.

The only logical problem I notice is that you're passing strings by value (it's a char** and reassigning it in the function will not affect the caller variable containing the pointer). Changing the signature to

void remove_stopwords(char**& strings, int* length)

so a reference is passed instead should fix it.

(1) Using std::vector<const char *> would seem more logical, even better an std::vector<std::string> if possible, that would take care of all allocations and deallocations.

score 2 · Answer 2 · answered May 22 '15 at 20:46

2

every C-style string was created using new.

I suspect this is your problem -- C style strings are char arrays, so you can't readily create them with new, you need to use new[]. Which means you need to use delete[].

answered May 22 '15 at 20:46

Chris Dodd

119,907
13
134
226

score 1 · Answer 3 · answered May 22 '15 at 21:20

As @6502 pointed out, your basic problem is fairly simple: you're passing a char **, and attempting to modify it (not what it points at) in the function.

You're using that as a dynamically allocated array of strings, so what you're modifying is just the copy of the pointer that was passed into the function. Since you (apparently) want the function to modify what was passed into it, you need to either pass a char *** (ugh!) or char **& (still quite awful).

You really should use a vector<std::string> for the data. At least in my opinion, the code to remove the stop words should be written as a generic algorithm, something on this general order:

template <typename InIt, typename OutIt>
void remove_stop_words(InIt b, InIt e, OutIt d) { 
    std::remove_copy_if(b, e, d, 
        [](std:string const &s) { is_stop_word(s); });
}

With this, the calling code would look something like this:

// read input
std::vector<std::string> raw_input { std::istream_iterator<std::string>(infile), 
                                     std::istream_iterator<std::string>() };

// Filter out stop words:
std::vector<std::string> filtered_words;

remove_stop_words(raw_input.begin(), raw_input.end(), 
                  std::back_inserter(filtered_words));

In a case like this, however, you don't really need to store the raw input words into a vector at all. You can pass an istream_iterator directly to remove_stop_words, and have it just produce the desired result:

std::ifstream in("raw_input.txt");

std::vector<std::string> filtered_words;

remove_stop_words(std::istream_iterator<std::string>(in), 
                  std::istream_iterator<std::string>(),
                  std::back_inserter(filtered_words));

As an aside, you could also consider using a Boost filter_iterator instead. This would/will allow you to do the filtering in the iterator as you read the data rather than in an algorithm applied to the iterator.

What is the exact behaviour of delete and delete[]?

3 Answers3