Покритикуйте C++ код - О работе

Здравствуйте, Тёмчик, Вы писали:

Тё>здесь

Ну вот сделал-таки работающий CMake. А где там C++ 14?

Point vertices[3];
...
Triangle::Triangle( Point offset, Point v1, Point v2, Point v3 )
        : offset(offset), vertices( { v1, v2, v3 } ) {

Вот это безобразие какой компилятор понимает?

class Point {
public:
    int x;
    int y;


public:
    Point(int x, int y);

    Point(Point const &other);
};

Нафига ~~POD структуре баян~~? https://en.cppreference.com/w/cpp/language/rule_of_three

private:
    void accept( ShapeVisitor & visitor ) const;

override? visitor можно убрать, типа достаточно.

shapes.emplace_back( new Circle( offset, 2 ) );

make_unique<Circle>(offset, 2)

for( auto i = shapes.begin(); i != shapes.end(); ++i, ++lineNum ) {

for( auto& shape : shapes)

Ну и так далее. Некогда ревьювить сейчас

Здравствуйте, ned, Вы писали:

ned>Здравствуйте, Тёмчик, Вы писали:

Тё>>здесь

ned>Ну вот сделал-таки работающий CMake. А где там C++ 14?
Ничего я не делал. Откопал в почте и перепаковал пару заданий. C++14 было в опциях проекта в CLion.

ned>

ned>Point vertices[3];
ned>...
ned>Triangle::Triangle( Point offset, Point v1, Point v2, Point v3 )
ned>        : offset(offset), vertices( { v1, v2, v3 } ) {
ned>

ned>Вот это безобразие какой компилятор понимает?
Ээээ C++ 14?

Оно как-то собиралось и крутилось под отладчиком

ned>

ned>class Point {
ned>public:
ned>    int x;
ned>    int y;


ned>public:
ned>    Point(int x, int y);

ned>    Point(Point const &other);
ned>};
ned>

ned>Нафига ~~POD структуре баян~~? https://en.cppreference.com/w/cpp/language/rule_of_three
Ага, слажал.

ned>

ned>private:
ned>    void accept( ShapeVisitor & visitor ) const;
ned>

ned>override? visitor можно убрать, типа достаточно.
ок. Это критично убирать, или вкусовщина?

ned>

ned>shapes.emplace_back( new Circle( offset, 2 ) );
ned>

ned>make_unique<Circle>(offset, 2)
так меньше копирования, чем с implicit конструктором unique_ptr? Просто в твоем каноничном варианте больше букв.

ned>

ned>for( auto i = shapes.begin(); i != shapes.end(); ++i, ++lineNum ) {
ned>

ned>for( auto& shape : shapes)
не знал, что так теперь можно.

ned>Ну и так далее. Некогда ревьювить сейчас

И за это спасибо

Здравствуйте, Тёмчик, Вы писали:

Тё>Оффтоп из ветки

Тё>здесь

Уровень выпускника-отличника, который не сильно разбирается в языке. Часть проблем, видимых невооруженным взглядом, уже указал ned.
Еще using namespace прямо в заголовочном файле.
Класс MultiPowerPoint сделан "на отвали":

* публичный атрибут slaves;
* методы turnOn/turnOff в своей реализации -- типичная копи-паста, легко устраняемая за счет вспомогательного закрытого метода, принимающего ext::ApplienceVisitor&.

Здравствуйте, Тёмчик, Вы писали:

Тё>Ээээ C++ 14?

Оно как-то собиралось и крутилось под отладчиком

А, gcc extension (parenthesized member array initialization). Лучше просто vertices{ v1, v2, v3 }.

Тё>ок. Это критично убирать, или вкусовщина?

Не, некритично. Масло масляное.

Тё>так меньше копирования, чем с implicit конструктором unique_ptr? Просто в твоем каноничном варианте больше букв.

Там move будет без копирования. В данном случае разницы никакой, но лучше new/delete в коде избегать.
А вообще вот: https://herbsutter.com/gotw/_102/

Здравствуйте, so5team, Вы писали:

S>Уровень выпускника-отличника, который не сильно разбирается в языке. Часть проблем, видимых невооруженным взглядом, уже указал ned.
Эээ только я уже очень стар и никогда не был отличником

S>Еще using namespace прямо в заголовочном файле.
уменьшил число букв. Это правда так плохо?

S>Класс MultiPowerPoint сделан "на отвали":

S>* публичный атрибут slaves;
ок

S>* методы turnOn/turnOff в своей реализации -- типичная копи-паста, легко устраняемая за счет вспомогательного закрытого метода, принимающего ext::ApplienceVisitor&.
Напишите код, чтоб пояснить. Ибо я ещё раз перечитал- ничего там ни отнять, ни прибавить.

Здравствуйте, Тёмчик, Вы писали:

S>>Еще using namespace прямо в заголовочном файле.
Тё>уменьшил число букв. Это правда так плохо?

По мне так это было самым плохим что было в коде. Не надо потому, что рано или подно приводит к адище с именами: https://stackoverflow.com/questions/8394393/c-stl-remove-error

Здравствуйте, ned, Вы писали:

ned>А, gcc extension (parenthesized member array initialization). Лучше просто vertices{ v1, v2, v3 }.
ок

ned>Не, некритично. Масло масляное.
Вообще я отвык от C++- потому так написал. Ещё и скобки странные т.к. в то время в команде было такое соглашение.

Тё>>так меньше копирования, чем с implicit конструктором unique_ptr? Просто в твоем каноничном варианте больше букв.

ned>Там move будет без копирования. В данном случае разницы никакой, но лучше new/delete в коде избегать.
ned>А вообще вот: https://herbsutter.com/gotw/_102/

// At some call site:
f( std::unique_ptr<T1>{ new T1 }, std::unique_ptr<T2>{ new T2 } );

У Саттера проблема нескольких аллокаций: первый обьект создан успешно, второй бросил- успешно созданный обьект утек. Переписать можно как явно инициировать первый unique_ptr, потом на другом шаге второй, потом позвать функцию. Либо использовать helper как ты предложил, который внутри сделает 1 аллокацию. Т.о. в моём коде проблемы утечки нет- остальное это соглашения.

Здравствуйте, Тёмчик, Вы писали:

Тё>уменьшил число букв. Это правда так плохо?

Да. Очень плохо.

S>>* методы turnOn/turnOff в своей реализации -- типичная копи-паста, легко устраняемая за счет вспомогательного закрытого метода, принимающего ext::ApplienceVisitor&.
Тё>Напишите код, чтоб пояснить. Ибо я ещё раз перечитал- ничего там ни отнять, ни прибавить.

class MultiPowerPoint {
  ...
  void forEachApply(ext::ApplienceVisitor & v) {
    for( auto & s : slaves )
      s->accept(v);
  }
  ...
public:
  void turnOn() { forEachApply(onVisitor); }
  void turnOff() { forEachApply(offVisitor); }
  ...
};

Тут еще напрашивается вопрос о том, имеет ли смысл вообще создавать onVisitor и offVisitor атрибутами в MultiPowerPoint. Возможно, их можно было бы создавать по-месту:

void turnOn() {
  ext::TurnOnVisitor on;
  forEachApply(on);
}

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

Здравствуйте, so5team, Вы писали:

Тё>>уменьшил число букв. Это правда так плохо?

S>Да. Очень плохо.
Ну хз. Очень плохо, значит очень плохо

.

S>>>* методы turnOn/turnOff в своей реализации -- типичная копи-паста, легко устраняемая за счет вспомогательного закрытого метода, принимающего ext::ApplienceVisitor&.
Тё>>Напишите код, чтоб пояснить. Ибо я ещё раз перечитал- ничего там ни отнять, ни прибавить.
S>

class MultiPowerPoint {
S>  ...
S>  void forEachApply(ext::ApplienceVisitor & v) {
S>    for( auto & s : slaves )
      s->>accept(v);
S>  }
S>  ...
S>public:
S>  void turnOn() { forEachApply(onVisitor); }
S>  void turnOff() { forEachApply(offVisitor); }
S>  ...
S>};

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

S>Тут еще напрашивается вопрос о том, имеет ли смысл вообще создавать onVisitor и offVisitor атрибутами в MultiPowerPoint. Возможно, их можно было бы создавать по-месту:
S>

void turnOn() {
S>  ext::TurnOnVisitor on;
S>  forEachApply(on);
S>}

да, согласен, упустил этот момент.

S>Ну и развивая мысль далее, имело ли вообще смысл связываться здесь с динамическими полиморфизмом. Или выгоднее сделать статический полиморфизм на шаблонах.
Double dispatch на шаблонах? Оно не переусложнение задачи?

Здравствуйте, Тёмчик, Вы писали:

Тё>А метод-то зачем?- плодить сущности без нужды нехорошо.

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

S>>Ну и развивая мысль далее, имело ли вообще смысл связываться здесь с динамическими полиморфизмом. Или выгоднее сделать статический полиморфизм на шаблонах.
Тё>Double dispatch на шаблонах? Оно не переусложнение задачи?

Зависит от задачи. Это в Java или в каком-нибудь Go у вас нет выбора. А в C++ он есть, и в зависимости от условий, приходится выбирать между динамическим полиморфизмом и статическим. У каждого есть свои преимущества и недостатки. А "переусложнение" -- это цена, которую придется заплатить, например, если мы хотим избежать использования виртуальных функций в рантайме.

Здравствуйте, Тёмчик, Вы писали:

Тё>У Саттера проблема нескольких аллокаций: первый обьект создан успешно, второй бросил- успешно созданный обьект утек. Переписать можно как явно инициировать первый unique_ptr, потом на другом шаге второй, потом позвать функцию.
Для этого и используют make_*, unique_ptr и try-catch. На эту тему тут Джаззер 100 лет назад целый объёмный пост делал к тогдашнему С++х03.

Здравствуйте, Kernan, Вы писали:

Тё>>У Саттера проблема нескольких аллокаций: первый обьект создан успешно, второй бросил- успешно созданный обьект утек. Переписать можно как явно инициировать первый unique_ptr, потом на другом шаге второй, потом позвать функцию.
K>Для этого и используют make_*, unique_ptr и try-catch. На эту тему тут Джаззер 100 лет назад целый объёмный пост делал к тогдашнему С++х03.

Проблема известная. К моему случаю не относится.

Здравствуйте, so5team, Вы писали:

S>Затем, что у вас концептуально есть такая задача: обойти все содержимое slave и применить к каждому элементу некий визитор. Вы кодируете решение этой задачи два раза в разных местах, что есть копипаста.
Но Вы скопипастили код от std::for_each и более того, породили ненужную сущность в классе. Вынесли бы тогда в utility-класс.

S>>>Ну и развивая мысль далее, имело ли вообще смысл связываться здесь с динамическими полиморфизмом. Или выгоднее сделать статический полиморфизм на шаблонах.
Тё>>Double dispatch на шаблонах? Оно не переусложнение задачи?

S>Зависит от задачи. Это в Java или в каком-нибудь Go у вас нет выбора. А в C++ он есть, и в зависимости от условий, приходится выбирать между динамическим полиморфизмом и статическим. У каждого есть свои преимущества и недостатки. А "переусложнение" -- это цена, которую придется заплатить, например, если мы хотим избежать использования виртуальных функций в рантайме.
Вы уходите от ответа на простой вопрос- нужно ли переусложнение с имплементацией double dispatch на шаблонах. Более того, паттерн visitor как раз придумали из за отсутствия встроенного в средства языка C++ и Java double dispatch.

Здравствуйте, Тёмчик, Вы писали:

Тё>Но Вы скопипастили код от std::for_each

Это только потому, что в отсутствии в C++ Ranges использование std::for_each потребовало бы больше кода.

Тё>и более того, породили ненужную сущность в классе. Вынесли бы тогда в utility-класс.

Это нужная сущность, и нужна она именно внутри класса, т.к. ей нужен доступ к slaves.
Нужна она потому, что кроме устранения копипасты метод forEachApply позволит вам более дешево поменять представление slaves. Если, скажем, вместо vector-а вы будете использовать vector<vector<...>>.

Utility-классы -- это Java головного мозга, сразу за дверь.

S>>Зависит от задачи. Это в Java или в каком-нибудь Go у вас нет выбора. А в C++ он есть, и в зависимости от условий, приходится выбирать между динамическим полиморфизмом и статическим. У каждого есть свои преимущества и недостатки. А "переусложнение" -- это цена, которую придется заплатить, например, если мы хотим избежать использования виртуальных функций в рантайме.
Тё>Вы уходите от ответа на простой вопрос- нужно ли переусложнение с имплементацией double dispatch на шаблонах. Более того, паттерн visitor как раз придумали из за отсутствия встроенного в средства языка C++ и Java double dispatch.

Мозги включите, если они есть. Все ответы были даны.

Здравствуйте, so5team, Вы писали:

S>Здравствуйте, Тёмчик, Вы писали:

Тё>>Но Вы скопипастили код от std::for_each

S>Это только потому, что в отсутствии в C++ Ranges использование std::for_each потребовало бы больше кода.
Скопипастили по факту из utility класса (ну хорошо, из юнита)

. Т.е. добавили несколько строчек говнокода.

Тё>>и более того, породили ненужную сущность в классе. Вынесли бы тогда в utility-класс.

S>Это нужная сущность, и нужна она именно внутри класса, т.к. ей нужен доступ к slaves.
Ей вообще-то не надо ничего знать про slaves. Ее задача- перебрать элементы коллекции и к каждому применить некое действие.

S>Нужна она потому, что кроме устранения копипасты метод forEachApply позволит вам более дешево поменять представление slaves. Если, скажем, вместо vector-а вы будете использовать vector<vector<...>>.
Не фантазируйте.

S>Utility-классы -- это Java головного мозга, сразу за дверь.
Вы прямо фундаментальный фанатик C++.

S>>>Зависит от задачи. Это в Java или в каком-нибудь Go у вас нет выбора. А в C++ он есть, и в зависимости от условий, приходится выбирать между динамическим полиморфизмом и статическим. У каждого есть свои преимущества и недостатки. А "переусложнение" -- это цена, которую придется заплатить, например, если мы хотим избежать использования виртуальных функций в рантайме.
Тё>>Вы уходите от ответа на простой вопрос- нужно ли переусложнение с имплементацией double dispatch на шаблонах. Более того, паттерн visitor как раз придумали из за отсутствия встроенного в средства языка C++ и Java double dispatch.

S>Мозги включите, если они есть. Все ответы были даны.
Если Вы не можете ответить на мой вопрос- не отвечайте. Почитайте литературу по Visitor. Не то чтобы я на него надрачивал- но в той конторе, для которой делал тест- незнание Visitor не прощают (общался с ними на митапе).

Здравствуйте, Тёмчик, Вы писали:

Тё>Скопипастили по факту из utility класса (ну хорошо, из юнита)

. Т.е. добавили несколько строчек говнокода.

И это говорит человек, который наговнокодил тоже самое дважды.

S>>Это нужная сущность, и нужна она именно внутри класса, т.к. ей нужен доступ к slaves.
Тё>Ей вообще-то не надо ничего знать про slaves. Ее задача- перебрать элементы коллекции и к каждому применить некое действие.

Ее задача перебрать элементы не какой-то коллекции, а конкретно slaves.

S>>Нужна она потому, что кроме устранения копипасты метод forEachApply позволит вам более дешево поменять представление slaves. Если, скажем, вместо vector-а вы будете использовать vector<vector<...>>.
Тё>Не фантазируйте.

Вы просто не знаете, чем опасна копипаста.

S>>Мозги включите, если они есть. Все ответы были даны.
Тё>Если Вы не можете ответить на мой вопрос- не отвечайте.

Ответ был дан. Если вы не можете понять то, что вам пишут, то это ваши проблемы.

Здравствуйте, Тёмчик, Вы писали:

Тё>Здравствуйте, Kernan, Вы писали:

Тё>>>У Саттера проблема нескольких аллокаций: первый обьект создан успешно, второй бросил- успешно созданный обьект утек. Переписать можно как явно инициировать первый unique_ptr, потом на другом шаге второй, потом позвать функцию.
K>>Для этого и используют make_*, unique_ptr и try-catch. На эту тему тут Джаззер 100 лет назад целый объёмный пост делал к тогдашнему С++х03.

Тё>Проблема известная. К моему случаю не относится.
Извиняюсь, что встреваю — вопрос показался интересным.
Не может ли вот этот код

[...]
mpp.slaves.emplace_back( new Oven() );
[...]

Превратиться в следующую последовательность?
— создали Oven в куче, держим сырой указатель;
— mpp.slaves.emplace_back вызывает grow до того как создан unique_ptr;
— получаем исключение от allocator;
— указатель на Oven потерян.

При ипользовании push_back() в данном случае проблемы не было бы — unique_ptr был бы создан до вызова самого push_back() и возможной реаллокации в контейнере.

Здравствуйте, migun, Вы писали:

M>Извиняюсь, что встреваю — вопрос показался интересным.
M>Не может ли вот этот код
M>

M>[...]
M>mpp.slaves.emplace_back( new Oven() );
M>[...]
M>

M>Превратиться в следующую последовательность?
M>- создали Oven в куче, держим сырой указатель;
M>- mpp.slaves.emplace_back вызывает grow до того как создан unique_ptr;
M>- получаем исключение от allocator;
M>- указатель на Oven потерян.

M>При ипользовании push_back() в данном случае проблемы не было бы — unique_ptr был бы создан до вызова самого push_back() и возможной реаллокации в контейнере.

Интересно. Получается, вот так выстрелил себе в ногу путём использования новой (для меня) фичи emplace_back, но неиспользования make_unique.

От:	Тёмчик	жж
Дата:	03.10.18 23:37
Оценка:

	От:	ned
	Дата:	04.10.18 03:05
	Оценка:	6 (1)

От:	Тёмчик	жж
Дата:	04.10.18 03:44
Оценка:

От:	so5team	https://stiffstream.com
Дата:	04.10.18 05:36
Оценка:

	От:	ned
	Дата:	04.10.18 05:46
	Оценка:

От:	kaa.python	РСДН профессионально мёртв и завален ватой.
Дата:	04.10.18 06:03
Оценка:	+2

От:	Kernan	https://rsdn.ru/forum/flame.politics/
Дата:	04.10.18 07:39
Оценка:

От:	migun	https://magnumbytes.com/
Дата:	04.10.18 12:58
Оценка:	3 (1) +1