Видите ли вы какие-либо проблемы с этим кодом C ++? - PullRequest
Новая
       4

Видите ли вы какие-либо проблемы с этим кодом C ++?

1 голос
/ 26 апреля 2011 
#include<iostream>
#include<conio.h>
using namespace std;
class A
{
public:
       int *p;      
A()
{
   p =new int;
}

~A()
{
delete p;   //Is this what i am doing is correct?
cout << "in A's destructor"<<endl;
}

};

int main()
{
A *obj=new A;
delete obj;    
getch();
}

Это программы, которые я выполнил в Dev c ++, компилирует и выполняет нормально. Но я сомневаюсь, что это не хорошо. Особенно в деструкторе, где я говорю delete P

я не прав?

Ответы [ 4 ]

11 голосов
/ 26 апреля 2011

Этот код логически хорош (часть с new / delete, о которой вы спрашиваете), но плохо спроектирован в других аспектах.

Во-первых, если class A владеет выделенной кучей int (int живет столько, сколько живет class A объект), нет смысла создавать его с new, это было бы намного эффективнее сделать его переменной-членом class A. Во-вторых, вы не должны делать это public - это нарушает инкапсуляцию и допускает много злоупотреблений.

Наконец, у вас все еще есть конструктор копирования и оператор присваивания, которые разрешены и не реализованы, поэтому каждый раз, когда объект класса копируется, вы получаете поверхностное копирование. Вы должны либо реализовать их надлежащим образом, либо запретить их .

Часть, в которой вы delete переменную, выделенную с помощью new, вполне подходит.

2 голосов
/ 26 апреля 2011

Я вижу 4 проблемы с этим кодом:

  • два использования new
  • два использования delete

Возможно, что урезанный сокращенный код в некоторой степени отсутствует по сравнению с оригинальной реализацией, или вы пришли из языка, в котором все написано new, но этот код далек от идиоматического C ++.

Идиоматическое переписывание: 

#include<iostream>

class A
{
public:
  int p;

  ~A() { std::cout << "~A" << std::endl; } // force a flush with endl
};

int main(int argc, char* argv[])
{
  {
    A obj;
  } // for destruction to occur before getch

  // use streams instead of getch
  char a;
  std::cin >> a;

  // the signature says it returns an int...
  return 0;
}
1 голос
/ 26 апреля 2011

Вам также нужно определить свой собственный конструктор копирования и оператор присваивания (или объявить их как закрытые, чтобы сделать класс некопируемым) в этом случае;конструктор копирования по умолчанию и оператор присваивания делают поверхностную копию.То есть они копируют указатель, а не содержимое указателя.Поэтому, если вы сделаете это: 

A x;
A y = x;

Оба x.p и y.p указывают на одно и то же место, следовательно, когда они уничтожены, они пытаются освободить одну и ту же память, что приводит к неопределенному поведению.

Чтобы исправить это, определите свой собственный конструктор копирования и оператор присваивания, который копирует int: 

class A
{
public:
        A() : p(new int) {}
        A(const A& obj) : p(new int(*obj.p)) {}
        ~A() { delete p; }
        A& operator=(const A& obj) { *p = *obj.p; }
private:
        int *p;
};
1 голос
/ 26 апреля 2011

Как указал острый зуб, использование delete в dtor полностью допустимо. delete null определяется в стандарте как noop.

однако, для подобных вещей рассмотрите использование shared_ptr или чего-то подобного ...

НТН

Mario

...