goto vs break & continue , Кроха-сын пришел к отцу и спросила кроха: "goto - это хорошо?"

Цитата OpenGL @ 12.07.20, 16:35

от проблем же, которые есть в твоём изначальном коде,

Погоди, а в моем коде уже проблемы появились?

Цитата JoeUser @ 13.07.20, 02:22

Погоди, а в моем коде уже проблемы появились?

Одну из них ты даже обнаружил уже тут на форуме

Добавлено 13.07.20, 07:58

Цитата JoeUser @ 12.07.20, 11:08

Забыл про cleanup соединения.

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

Добавлено 13.07.20, 08:23

Цитата JoeUser @ 11.07.20, 13:32

struct addrinfo *addrs;
...
ret = getaddrinfo(Host.toLocal8Bit().data(), NULL, &hint, &addrs);

Память под указатель выделяется, но нигде не освобождается - утечка.
Вот посмотри как ее используют:
https://man7.org/linux/man-pages/man3/getaddrinfo.3.html

Добавлено 13.07.20, 08:29
А если бы ты не придумывал вот эти вот говновелосипеды с goto, а везде использовал RAII, хоть так, как тебе выше написали (не должно быть у тебя в коде сырых указателей от слова вообще, а у тебя их целая телега) то у тебя бы и утечек не было. А так тут еще можно найти ошибки, если внимательно взглянуть, да всякие подводные камни выползут. И читать портянку такого кода не очень приятно. Тут пол функции - мусора из всяких goto, из за чего тело функции увеличилось в объеме в три раза, оно стало не читабельным, и стремным, кладезью ошибок.

Сообщение отредактировано: Wound - 13.07.20, 08:30

Цитата Wound @ 13.07.20, 07:55

Одну из них ты даже обнаружил уже тут на форуме

RAII тут не помощник! Как с goto, так и с деструктором - можно забыть про необходимость закрытия соединения. Какая разница где забывать?

Цитата Wound @ 13.07.20, 07:55

Память под указатель выделяется, но нигде не освобождается - утечка.

Вот за это спасибо! Честно говоря понадеялся на 100+ спасибок на один из ответов на SO, и не перепроверил. Но и тут мне RAII бы не подсказал вызвать freeaddrinfo. Так что если бы у нас была тема "RAII vs Киля" - я бы только за тебя и топил бы

Цитата JoeUser @ 13.07.20, 10:28

RAII тут не помощник! Как с goto, так и с деструктором - можно забыть про необходимость закрытия соединения. Какая разница где забывать?

Уже был спор на эту тему. Как говорил OpenGL - в С++ утечек нет. RAII - тут как раз таки помощник. Почитай про то, что это такое. Тебе сразу станет ясно(потому что хоть ты когда то и читал про это, но видно что ты плаваешь в этом).
Во первых - если ты юзаешь указатель - и оборачиваешь его в класс/unique_ptr/shared_ptr/etc - в любом случае тебе придется задуматься что писать в деструкторе/делитере, либо за тебя это сделает смарт поинтер.
Во вторых - ты пишешь обертку для ресурса, соответственно забыть написать освобождение - довольно не тривиальная задача, ну разве что ты будешь писать не думая вообще.

Цитата JoeUser @ 13.07.20, 10:28

Вот за это спасибо! Честно говоря понадеялся на 100+ спасибок на один из ответов на SO, и не перепроверил. Но и тут мне RAII бы не подсказал вызвать freeaddrinfo. Так что если бы у нас была тема "RAII vs Киля" - я бы только за тебя и топил бы

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

fingerprint = libssh2_hostkey_hash(session, LIBSSH2_HOSTKEY_HASH_SHA1);

fingerprint - у тебя const char*, тот же указатель! Может быть там лучше было бы юзнуть std::string? Да и вообще зачем тебе этот fingerprint, он у тебя вообще нигде не используется, только в этой строке.

Цитата Wound @ 13.07.20, 11:01

Уже был спор на эту тему. Как говорил OpenGL - в С++ утечек нет.

1 момент) Я тебе по вызов процедуры закрытия сокета // int shutdown(int sockfd, int how); //, а ты мне про утечки!
2 момент) Как мне RAII напомнит вызвать freeaddrinfo, тем более что выделял не я, а либа winsock2?

Добавлено 13.07.20, 11:55

Цитата Wound @ 13.07.20, 11:01

fingerprint - у тебя const char*, тот же указатель!

Это скорее всего указатель на кишки libssh2, и они освободятся когда я вызову libssh2_exit();

Цитата JoeUser @ 13.07.20, 11:52

1 момент) Я тебе по вызов процедуры закрытия сокета // int shutdown(int sockfd, int how); //, а ты мне про утечки!

Так и я тебе про это, только там наверное close использовать нужно, хотя я хз, в примере используют close: https://man7.org/linux/man-pages/man3/getaddrinfo.3.html но не суть важно.

Цитата JoeUser @ 13.07.20, 11:52

2 момент) Как мне RAII напомнит вызвать freeaddrinfo, тем более что выделял не я, а либа winsock2?

Ну а что ты когда пользуешься функциями/классами - ты по ним вообще доку не читаешь? Потом, зачем тебе юзать RAII - если у тебя не нужно освобождать ресурс руками? Я тебе про это и пишу.

Если ты начинаешь оборачивать ресурс в RAII Обертку - значит он не может сам по себе освободится, для этого RAII и нужен. Ты берешь функцию getaddrinfo, посмотри внимательно на последний ее параметр, он тебя нисколько не смущает? Меня он как раз и смутил:

ret = getaddrinfo(Host.toLocal8Bit().data(), NULL, &hint, &addrs);

addrs - у тебя неинициализированный указатель, а ты берешь его адрес и передаешь последним параметром в эту функцию, значит тут возможны два варианта - либо ты выхватишь Access Violation прямо внутри этой функции, либо она тебе вернет выделенную память(потому что если функция принимает указатель на указатель, и внутри работает с ним, не выделяя для него память - значит это AV, но обычно когда функция принимает указатель на указатель - практически всегда(ну кроме там исключительных случаев, например там работа с матрицами, двумерными массивами и т.д.) - это делается для того, чтоб выделить память и вернуть ее наружу, т.к. в С/С++ по умолчанию семантика значений, т.е. передай ты просто указатель - будет утечка), лезем в доку: https://man7.org/linux/man-pages/man3/getaddrinfo.3.html
И что мы видим?

Цитата

int getaddrinfo(const char *node, const char *service,
const struct addrinfo *hints,
struct addrinfo **res);

void freeaddrinfo(struct addrinfo *res);

const char *gai_strerror(int errcode);

Т.е. есть функция освобождения выделенного ресурса, соответственно ее и юзаем для освобождения памяти.
Когда в С работаешь - там нет классов и деструкторов, там все нужно руками освобождать, если не сказано обратное. Соответственно если есть функция, которая возвращает тебе ресурс, логично предположить что должна быть функция освобождения ресурса. В Си по другому не работают.
Допустим в apache есть функции для работы с ним, которым не нужно ресурсы руками освобождать, он сам их освобождает, там об этом прямо сказано, но там и типы соответствующие используются, а не голые указатели.

Ты когда у себя где то пишешь T* ptr = new T - ты потом память не освобождаешь что ли?

Цитата JoeUser @ 13.07.20, 11:52

Это скорее всего указатель на кишки libssh2, и они освободятся когда я вызову libssh2_exit();

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

Сообщение отредактировано: Wound - 13.07.20, 12:31

Цитата JoeUser @ 13.07.20, 02:22

Погоди, а в моем коде уже проблемы появились?

Я их описывал вообще-то. Сложное сопровождение кода, сложное понимание, и да - утечки ресурсов, т.к. если управление всеми ресурсами отдавать деструкторам, то от утечек это избавляет практически на 100%.

Цитата Wound @ 13.07.20, 12:25

Так а какой тогда смысл в переменной fingerprint ?

Не знаю, и я об этом спрашивал на SO - тоже молчат. Если это обязательная последовательность диалога с SFTP, она должна быть в коде. А если нет - можно выбросить. Но я так и не нашел норм описание SFTP, т.к. RFC на него нету, а есть только draft-доки от IETF.

Цитата OpenGL @ 13.07.20, 12:33

управление всеми ресурсами отдавать деструкторам

Ага и в деструкторах главное не забыть написать нужную функцию освобождения библиотечного ресурса. В плане с goto - та же проблема не забыть.

Добавлено 13.07.20, 14:24

Цитата Wound @ 13.07.20, 12:25

Ты когда у себя где то пишешь T* ptr = new T - ты потом память не освобождаешь что ли?

Всегда освобождаю. Но в Qt есть есть свой механизм, когда удалять не нужно, даже противопоказано. Пример в конструкторе:

...
QPushButton *B = new QPushButton("Акей");
QVBoxLayout *L = new QVBoxLayout();
L->addWidget(B);
setLayout(L);

L->addWidget(B); - L запоминает потомка B, перед своим удалением - он его удалит
setLayout(L); - текущий виджет запоминает потомка L, перед своим удалением - он его удалит

Таким образом, мне в деструкторе делать ничо не нужно.

Цитата JoeUser @ 13.07.20, 14:10

Не знаю, и я об этом спрашивал на SO - тоже молчат. Если это обязательная последовательность диалога с SFTP, она должна быть в коде. А если нет - можно выбросить. Но я так и не нашел норм описание SFTP, т.к. RFC на него нету, а есть только draft-доки от IETF.

В конкретно данном случае - если ты выкинешь из кода fingerprint - это вообще ни на что не повлияет. Это ясно даже без доки.

Цитата JoeUser @ 13.07.20, 14:10

Ага и в деструкторах главное не забыть написать нужную функцию освобождения библиотечного ресурса. В плане с goto - та же проблема не забыть.

Я же пишу - это не тривиальная задача забыть в деструкторе написать освобождение. А вот с goto - это тривиальная задача. Обертку тебе нужно писать ровно 1 раз, потом раз ты в конструкторе выделяешь ресурс, значит если деструктор будет отсуствовать или он будет пустой - уже повод задуматься что ты вообще делаешь и нахрена ты такое пишешь.

Добавлено 13.07.20, 14:32

Цитата JoeUser @ 13.07.20, 14:10

L->addWidget(B); - L запоминает потомка B, перед своим удалением - он его удалит
setLayout(L); - текущий виджет запоминает потомка L, перед своим удалением - он его удалит

Таким образом, мне в деструкторе делать ничо не нужно.

значит говно этот ваш QT, раз он смешивает семантику языка с логикой. В данном случае у тебя сырые указатели, я не понимаю - как он их удалит? Тут RAII и не пахнет. Разве что он как JAVA периодически подчищает ресурсы, но это уже сродни языку, а не фреймворку или что это вообще?

Я бы понял вот такой пример:

QPushButton B = new QPushButton("Акей");
QVBoxLayout L = new QVBoxLayout();
L->addWidget(B);
setLayout(L);

В таком виде - все ок, никаких неоднозначностей нет. А вот конкретно в том виде, в котором написал ты - у меня возникает много вопросов. Либо ты не верно понимаешь работу QT.

Добавлено 13.07.20, 14:34
Вот к слову тут пишут что надо удалять ручками, либо юзать обертки: https://forum.qt.io/topic/80357/should-i-de...ocal-function/3

Добавлено 13.07.20, 14:37
Да и это было бы довольно странно как ты написал. Кинь пруфы на документацию, что оно работает так, как ты пишешь. Потому что у меня закрадываются сомнения. Как оно понимает что я работаю с указателем и что в конкретном месте нужно освободить из под него память? Это анриал, это уже какая то Java получается, но даже там не знают когда надо освобождать память, поэтому собственно С++ еще и существует как язык.

Добавлено 13.07.20, 14:39
А если я напишу:

int* pI = new int;

Оно мне тоже память почистит? Если нет, то в чем этот код отличается от:

Цитата JoeUser @ 13.07.20, 14:10

QPushButton *B = new QPushButton("Акей");
QVBoxLayout *L = new QVBoxLayout();
L->addWidget(B);
setLayout(L);

???
Я больше склоняюсь к тому, что ты ошибся. Не может так быть.

Добавлено 13.07.20, 14:44
Хотя если он там у себя внутри делает какой нибудь delete *this, может и будет работать(я уже два года на плюсах не писал, немного забывается), но всеравно это говноподход ИМХО, таким подходом всякие COM страдали на первых этапах, и то для них давно написаны RAII Обертки типа там ComPtr

Сообщение отредактировано: Wound - 13.07.20, 14:54

Цитата JoeUser @ 13.07.20, 14:10

Ага и в деструкторах главное не забыть написать нужную функцию освобождения библиотечного ресурса. В плане с goto - та же проблема не забыть.

Забыть написать освобождение ресурса во время написания RAII обёртки для этого ресурса это сильно

Цитата Wound @ 13.07.20, 14:28

А если я напишу:

int* pI = new int;

Оно мне тоже память почистит? Если нет, то в чем этот код отличается от:

Цитата JoeUser @ 13.07.20, 14:10

QPushButton *B = new QPushButton("Акей");
QVBoxLayout *L = new QVBoxLayout();
L->addWidget(B);
setLayout(L);

???

Тем, что в случае Qt это не обязательно ошибка. Передавая ресурс в нутря Qt, ты передаёшь ему владение, согласно договорённости с Qt, тогда как присваивая поинтер на хип сырому указателю, никому ничего не передаёшь и по-прежнему сам отвечаешь за энтот поинтер. Сравни:

std::shared_ptr<int> pI = new int;

Сообщение отредактировано: Qraizer - 13.07.20, 18:43

Как хорошо в D, в котором есть scope(exit). И не нужно ни goto ни бестолковых классов.

Сообщение отредактировано: applegame - 13.07.20, 18:49

Цитата Qraizer @ 13.07.20, 18:40

Тем, что в случае Qt это не обязательно ошибка. Передавая ресурс в нутря Qt, ты передаёшь ему владение, согласно договорённости с Qt, тогда как присваивая поинтер на хип сырому указателю, никому ничего не передаёшь и по-прежнему сам отвечаешь за энтот поинтер. Сравни:

Так я и говорю - смешение семантики языка с логикой выходит.

Когда ты пишешь вон как с std::shared_ptr - то в данном случае у тебя во первых разные типы данных с лева и с права.
Попробуй тогда уж написать по аналогии с QT:

std::shared_ptr<int>* pI = new std::shared_ptr<int>();

Потому как с std::shared_ptr - сразу видно, что тут используется враппер, а когда ты пишешь:

QPushButton *B = new QPushButton("Акей");

Тут из этого вообще никак не очевидно что ты пишешь враппер. Я во первых бы обернул это по привычке в std::unique_ptr, ну или на крайняк дальше по коду вызвал бы -> delete B;, а если дальше бы по коду оно упало у меня, то справедливо бы обматерил разрабов QT.
А в данном случае что происходит? Оно внутри делает delete *this?
Во первых это вообще не очевидно, во вторых - пописав годик вот на таком, ты по привычке везде будешь так писать, забыв вызвать delete, да и RAII тебе в таком случае нафиг не нужно, ты же привык что у тебя память сама освобождается. По мне так очень стремная практика. Даже в COM - и то юзаются врапперы.

Сообщение отредактировано: Wound - 13.07.20, 18:54

Цитата Wound @ 13.07.20, 14:28

Я больше склоняюсь к тому, что ты ошибся. Не может так быть.

Может. Ибо разрабы в QObject встроили звездолет, и для этого придумали свой дополнительный инструментарий - moc. Вот он-то и занимается тем, что на основе метаинформации начинает творить "чудеса".

И тем не менее, если написать new от класса-не потомка QObject - автоматического удаления не будет, ибо moc это не обрабатывает. Это по твоему примеру:

Цитата Wound @ 13.07.20, 14:28

А если я напишу:

int* pI = new int;

Добавлено 13.07.20, 20:01

Цитата Wound @ 13.07.20, 14:28

Вот к слову тут пишут что надо удалять ручками, либо юзать обертки: https://forum.qt.io/topic/80357/should-i-de...ocal-function/3

Тут правильно пишут. Ибо если я напишу вот так:

QPushButton *B = new QPushButton("Акей");
QVBoxLayout *L = new QVBoxLayout();
// без следующих двух строчек
// L->addWidget(B);
// setLayout(L);

Будет утечка памяти, т.к. переменные B и L никому во владение не передаются. В таком случае, там же правильно предлагают использовать QScopedPointer. Но что объявление кнопки и ее неиспользование, что размещение QString в куче и отсутствие передачи указателя куда-то, кто удалит - это дичь.

Добавлено 13.07.20, 20:06

Цитата OpenGL @ 13.07.20, 17:17

Забыть написать освобождение ресурса во время написания RAII обёртки для этого ресурса это сильно

Ну а чо? Я вон при cleanup сокета забыл сделать shutdown ему. По описалову - моя программа падать не будет, и система падать не будет. Будет только разница в поведении ранее созданного соединения, которое без shutdown будет по прежнему принимать TCP-пакеты, а в ответ молчать.

Цитата Wound @ 13.07.20, 14:28

Кинь пруфы на документацию, что оно работает так, как ты пишешь.

Лень доки рыть. Кину на краткое описание механизма, там вроде понятно расписали - "Qt владение «объектами» (T * и const T *)"