В чем ошибка в исходном коде, и есть ли она?

Question

[4ainik]

Здравствуйте. Есть простой исходный код:

class TLibrary{
    HINSTANCE hModule;
    TLibrary( const TLibrary &){}
    TLibrary(){}
public:
    TLibrary(const char *LibName){
        if( ( hModule = LoadLibrary(LibName) ) == NULL )
            throw Exception("Can't load library");
    }
    virtual FARPROC getProcAddress(const char *ProcName){
        FARPROC ptr = GetProcAddress(hModule, ProcName);
        if( ptr == NULL )
            throw Exception("Can't find func");
        return ptr;
    }
    virtual ~TLibrary(){
        FreeLibrary(hModule);
    }
};

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

http://cppcheck.sourceforge.net/cgi-bin/democlient.cgi
Cppcheck 1.84

[test.cpp:3]: (warning, inconclusive) Member variable 'TLibrary::hModule' is not initialized in the constructor.
[test.cpp:6]: (style) Class 'TLibrary' has a constructor with 1 argument that is not explicit. Such constructors should in general be explicit for type safety reasons. Using the explicit keyword in the constructor means some mistakes when using the class can be avoided.
[test.cpp:1]: (warning) The class 'TLibrary' has 'copy constructor' but lack of 'operator='.
Done!

Есть ли здесь специалисты, которые могли бы объяснить на простом родном языке, в чем же тут проблема?

Comments

[4ainik]

В C++ всё (почти) необходимо явно инициализировать, а то потом можно получить очень интересные проблемы.

Ну тут это можно сказать практически о любом языке и о любой программе. Только задумка была, запретить вызов конструктора копирования и в случае если бы были бы предприняты такие попытки вызовов, то компилятор бы все равно не смог ее скомпилировать, не так ли?! А так да, я полностью тут согласен, что в случае вызова конструктора копирования начальное значение члена hModule практически не предсказуемо. И мне знакомы разные случаи, от случайных значений до нуля.

К конструкторам с одним аргументом лучше добавлять explicit, чтобы избежать неявных преобразований и всяких интересных эффектов по ходу работы программы.

Вот тут можно подробнее, про интересные эффекты, уж больно они надоели :)

Золотое правило: сами реализовали деструктор, конструктор копии, оператор копирования, конструктор перемещения или оператор перемещения, задумайтесь о том, что вам возможно надо реализовать все пять методов.

Вот тут пожалуйста подробнее, чтобы было понятно не только мне, но и подрастающему поколению.

[jcmvbkbc]

Вот тут пожалуйста подробнее

Правило трёх/пяти.

[Vitaly]

Ну тут это можно сказать практически о любом языке и о любой программе.

Может быть. В C++ в неинициализированной переменной может быть любой случайный мусор, что приводит к плохим последствиям.

Вот тут можно подробнее, про интересные эффекты, уж больно они надоели :)

Может это прозвучит немного грубо, но читайте книги/статьи. Всё уже давно расписали.

А по поводу того, как запрещать конструкторы/операторы, тоже всё уже давно написано. В C++11 и выше есть ключевое слово delete. Уже написано даже, почему надо размешать методы в секции public и маркировать их delete, а не добавлять в private.

[4ainik]

Vitaly, да это все хорошо, я знаю что интернет бесконечен и в нем море полезной информации, только вот один пишет одно, второй другое и каждый прав по своему. Послушаешь одного и потом пожалеешь, не послушаешь и может даже никогда не пожалеешь...

Сама идеология каких-то там конструкторов по умолчанию порочна и то что придумали какой-то новый костыль в виде мудрецкого ключевого слова (причем не самого удачного) говорит о многом. Это ключевое слово может быть нормально переварено в последней студии, но вы не поверите сколько в природе существует еще других компиляторов. При этом их никто и никогда не будет уже модифицировать...

[Vitaly]

4ainik,

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

Разумеется ничего не стоит слепо принимать на веру. Есть критическое и аналитическое мышление? Используйте. Если нет, приобретайте. Кроме того, вам никто не мешает читать хорошие книги и экспериментировать.

Сама идеология каких-то там конструкторов по умолчанию порочна и то что придумали какой-то новый костыль в виде мудрецкого ключевого слова (причем не самого удачного) говорит о многом.

Не могли бы вы объяснить, о чём идёт речь и на чём основаны ваши выводы?

Это ключевое слово может быть нормально переварено в последней студии, но вы не поверите сколько в природе существует еще других компиляторов. При этом их никто и никогда не будет уже модифицировать...

Студия довольно популярный компилятор, но не самый удачный, на мой взгляд, т.к. местами сильно вольно трактует стандарт C++, да и полная поддержка новых стандартов там не так быстро появляется. Из популярных и более-менее массово используемых компиляторов есть ещё gcc, clang и intel. Все они в полной мере поддерживают C++11 (а некоторые уже и 14/17), за 7 лет-то можно реализовать было.
Если вам приходится компилировать чем-то экзотическим, то тут да, скорее всего могут быть проблемы. А компиляторы, поддержка которых прекращена, не слишком нужны.

[Василий Мельников]

На тему того что надо реализовывать (когда-то всю тройку, теперь пятерку :) )

#include <vector>
#include <iostream>
#include <algorithm>

class A
{
public:
	int* p;

	A() : p() {}

	A(int _) : p(new int(_)) {}

	A(const A& a) {
		p = new int(*a.p);
	}

	virtual ~A() {
		delete p;
	}
};

void dump(const A& a)
{
	std::cout << *a.p << std::endl;
}

int main()
{
	std::vector<A> v;
	v.push_back(A(1));
	v.push_back(A(2));
	v.push_back(A(3));
	
	v.erase(v.begin());

	std::for_each(v.begin(), v.end(), dump);
	
	// Вывод в косоль:
	// 2
	// 94734286 (мусор)
	// Хотя ожидалось
	// 2
	// 3
	// Что случилось?
	// Как исправить?
	
	std::cin.get();
	return 0;
}

[4ainik]

Я теряюсь в догадках, и честно говоря я и в тройке "плаваю", а уж про пятерку вообще недавно услышал, и я считаю что это все порочная практика, такими темпами, эта обязаловка будет расти в геометрической прогрессии, а по сути ничего не изменит. Может у вас уже есть шаблон класса вида:

template <class T>
class class_shablon{
T some_resource;
public:
class_shablon() : some_resource(0){
}

class_shablon(class_shablon &old_copy) : some_resource(old_copy.some_resource) {

}
... остальные сколько их там?? :)
}

[4ainik]

Василий Мельников, попробовал ваш код, ошибку не нашел, но обнаружил другой странный момент:
1) во-первых убрав v.erase(v.begin()); получил вывод 1,2,3 - что в принципе логично.
2) добавил печать из методов/констукторов класса и понял, что на банальных строчках: ( v.push_back(A(1)); v.push_back(A(2)); v.push_back(A(3)); ) происходит дикий оверхед, вызовы/перевызовы констукторов копирования/деструкторов, решил добавить после std::vector<A> v;
строку v.resize(4); и что интересно за каким-то программа рухнула именно на
v.resize(4);, т.е. после попадания в конструктор копирования, чего в принципе быть не должно!, т.к. по моим понятиям контейнер же пустой и не содержит не одного элемента!!! С какого идут вызовы констуктора копирования и что и куда копируется?
т.е. вот этот код уже не рабочий:

std::vector<A> v;
v.resize(4);

а я еще ничего никуда не добавил и даже ничего не печатал :)

[Василий Мельников]

4ainik, тут еще многое зависит от компилятора, варианта сборки, релиз, дебуг, оптимизация. Но смысл в том, что в классе не хватает оператора присваивания. Точнее, он есть но создан автоматически и просто побитно переносит все данные класса. В итоге получается что указатель копируется, а для "старого" или временного объекта вызывается деструктор и указатель указывает в никуда.
А в новых версиях C++ есть еще и конструкторы переноса и операторы такие же, про них тоже не стоит забывать.

Answer 1

[Алексей Гриченко]

Я так понимаю, что это С++ (луше было бы именно его в тегах поставить).

Судя по коду, вы хотите просто спрятать пару конструкторов. Для этого достаточно объявить их в приватной части класса, но не обязательно определять. Исправьте на такое, и будет вам счастье:

TLibrary( const TLibrary &);
TLibrary();

Comments

[4ainik]

Да, именно. Я и хотел спрятать эти конструкторы.
Только вот если пойти дальше и например начать расширять этот класс, или на минутку представить, что он делает что-то другое, и вот жизненно необходимо реализовать вот тот самый оператор присваивания, то как должно выглядеть ПРАВИЛЬНО объявление этого оператора "operator=" и какова должна быть его реализация? Какие еще подводные камни и грабли здесь прячутся?

Answer 2

[Vitaly]

[test.cpp:3]: (warning, inconclusive) Member variable 'TLibrary::hModule' is not initialized in the constructor.

В C++ всё (почти) необходимо явно инициализировать, а то потом можно получить очень интересные проблемы. В конструкторе необходимо инициализировать все поля класса в порядке объявления. Начиная с C++11 можно инициализировать по умолчанию через равно в момент объявления.

[test.cpp:6]: (style) Class 'TLibrary' has a constructor with 1 argument that is not explicit. Such constructors should in general be explicit for type safety reasons. Using the explicit keyword in the constructor means some mistakes when using the class can be avoided.

К конструкторам с одним аргументом лучше добавлять explicit, чтобы избежать неявных преобразований и всяких интересных эффектов по ходу работы программы.

(warning) The class 'TLibrary' has 'copy constructor' but lack of 'operator='.

Золотое правило: сами реализовали деструктор, конструктор копии, оператор копирования, конструктор перемещения или оператор перемещения, задумайтесь о том, что вам возможно надо реализовать все пять методов.

Comments

[Vitaly]

Вообще ваш код должен проще выглядеть с RAII:

class TLibrary
{
public:
    TLibrary(const char *LibName)
        : m_handle(LoadLibrary(LibName), &FreeLibrary)
    {
        if (!m_handle)
            throw Exception("Can't load library");
    }

    virtual FARPROC getProcAddress(const char *ProcName)
    {
        if (auto addr = GetProcAddress(m_handle.get(), ProcName))
            return addr;

        throw Exception("Can't find func");
    }

private:
    std::unique_ptr<HINSTANCE, decltype(&FreeLibrary)> m_handle; // shared_ptr?
};

Сразу говорю, я это не компилировал :)

Answer 3

[Василий Мельников]

Зависит от того как этот класс будет использоваться. Но потенциально ошибки есть:
1. При копировании объекта класса, исходный объект и копия будут иметь одно и тоже значение hModule
И если двойное освобождение может и прокатит относительно безболезненно, то вызов функции для освобожденного хендла уже нет.
2. Исключение из конструктора. Тут ни к чему плохому не приведет, но потенциально при расширении функционала и как подход вообще - весьма плох
3. То что поле класса потенциально может быть непроинициализировано, тоже понятно.

Comments

[4ainik]

вот такой вариант

class TLibrary{
    HINSTANCE hModule;
    TLibrary( const TLibrary &){
            throw Exception("Not allowed");
	}
    TLibrary(){
            throw Exception("Not allowed");
	}
public:
    explicit TLibrary(const char *LibName) : hModule(0){
         if( ( hModule = LoadLibrary(LibName) ) == NULL ){
			throw Exception("Can't load library");
		}
    }
	virtual FARPROC getProcAddress(const char *ProcName){
		FARPROC ptr = GetProcAddress(hModule, ProcName);
		if( ptr == NULL ){
			throw Exception("Can't find func");
		}
        	return ptr;
    }
    virtual ~TLibrary(){
		if(hModule){
        		FreeLibrary(hModule);
		}
    }
};

лучше? Т.е. предполагается что даже если каким-то образом будет попытка вызвать конструктор копирования, которая запрещена и не поддерживается, то код вызовет исключение.

Вообще для конструктора копирования размещение его в private/protected области должно же привести к ошибке еще на этапе компилирования?

[Василий Мельников]

Исключение их конструктора лучше не бросать. Т.к. в этом случае объект не создается и деструктор не вызывается. Просто "плохая привычка"
Не хватает оператора присваивания защищенного.
Если компилятор позволяет, тор чтобы анализатор отстал моно прямо при объявлении обнулить хендл.