CODEWITHSUNDEEP
c++arraysnew-operatordelete-operator
user1861248
user1861248

Reputation: 79

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

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.

Upvotes: 1

Views: 128

Answers (3)

Jerry Coffin
Jerry Coffin

Reputation: 490148

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.

Upvotes: 1

6502
6502

Reputation: 114491

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.

Upvotes: 6

Chris Dodd
Chris Dodd

Reputation: 126223

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[].

Upvotes: 2

Related Questions