Closed ghost closed 5 years ago
jkollss
commented
7 years ago Хочу заметить, что на простых типах замена
for(int i=0; i<32; i++)
на
for(int i=0; i<32; ++i)
не более, чем плацебо.
В этом легко убедиться, сгенерировав ассемблерный листинг опцией g++ -S или воспользоваться objdump. Различий в коде в современных компиляторах не будет.
ghost
commented
7 years ago Спасибо! Проверено: да, действительно. Впрочем, когда код пишется с самого начала, привычка к преинкременту полезнее (особенно при работе с другими языками, которые могут вести себя по другому). Можно спросить, был ли смысл в использовании конструкций чистого Си, а так же старого стандарта С++, или это может быть переписано на С++ вообще, и, возможно С++11 в частности?
xintrea
commented
7 years ago Здравствуйте!
Я ценю вашу работу, сделано много. Но в таком виде, к сожалению, принять не могу. Вы собрали в один реквест здоровенный кусок изменений, из которых реально необходимы процентов 15. Лучше было бы перед этой работой пообщаться со мной, чтоб стало понятно, что реально нужно для проекта, а что нет. Пожалуйста, не принимайте эту критику близко к сердцу, я просто пишу из соображений сопровождения кода.
Я вам скажу, где оптимизация действительно нужна: в методах загрузки XML и в методах выгрузки XML, но не принося в жертву удобство существующего сейчас кода. Единственное что оправдано в ваших изменениях - оптимизация кеширующими значениями границы цикла в моделях, но смотри пункт 3.
+ QString num = QString::number(rand());
+ QString result;
+ result.reserve(9+num.length());
+ result.append("image");
+ result.append(num);
+ result.append(".png");
+ return result;Видимо, для компьютера. Но я, как человек, не могу понять, зачем превращать одну простую команду в месиво команд с магической константой, тем более что в этом месте оптимизация не нужна.
Важно. Вы тестировали программу после того, как внесли изменения? Там есть места, когда кеширование границы цикла приведет к некорректному алгоритму.
Выражения вида "переменная++" для целочисленных типов уже давно оптимизируются компиляторами. Никакого смысла каверкать естественную запись Объект...Действие на обратную Действие...Объект нет. Ее читать и понимать неудобно.
Какой резон заменять запись приведения типа
(ClipboardBranch *)subbranch
на плохочитаемую
qobject_cast<ClipboardBranch *>(subbranch) ?
jkollss
commented
7 years ago По п.1 - совершенно согласен. Это называется преждевременная оптимизация и только вредит (с) Д.Кнут.
По п.2 - тоже согласен, переделано на какой-то абсурд. Я бы написал так:
return QString("image%1.png").arg(rand());
По п.3 - тоже согласен, экономия на спичках, которая выльется в проблемы.
По п.4 - я уже сказал выше. Префиксные операции хуже для читаемости.
По п.5 - не согласен. Устаревшее приведение типов типа (ClipboardBranch *)subbranch - потенциальный источник проблем. Нужно использовать static_cast, dynamic_cast и крайне осторожно reinterpret_cast и const_cast. Ну в Qt - qobject_cast.
Статический анализатор LLVM на конструкции прямого приведения типов уже давно ругается матом и правильно делает.
Ну и коммит должен быть атомарным и решать ровно одну проблему. По возможности конечно.
ghost
commented
7 years ago Здравствуйте!
Признаю, часть изменений, например постинкремент на преинкремент были перебором. И сделаны в процессе первого общего знакомства с проектом и Вашей реакцией на изменения. Придерживаюсь мнения, что десяток итераций в интерфейсе может и не первостепенная задача, но лучше пусть будет оптимизирована, чем оставлено в прежнем виде. (По поводу малого количества итераций в целом соглашусь).
Считаю идеальным, когда код написан одновременно и для компьютера, и для человека, в зависимости от конкретной ситуации чуть больше отдавая предпочтение одному или другому. И в ряде случаев приоритет одного над другим может быть обусловлен либо личными взглядами, либо вопросами экономической эффективности (последнее в данном случае отсутствует). Фактически каждая операция "+" со строками создаёт лишний объект QString, выделяет память и копирует данные. В приведённом примере 2 раза: return "image"+QString::number(rand())+".png"; (Насколько это оптимизируется на стадии компилирования - большой вопрос. И это может оказаться вдвойне осмысленным, при использовании этого кода на платформе Android. Альтернативный способ улучшить ситуацию был бы с помощью DEFINES += QT_USE_QSTRINGBUILDER в файле проекта.) На мой взгляд, это был случай, когда можно поднять производительность, лишь незначительно уменьшив читаемость. Причина появления магической константы: отсутствие смысла явного создания QString для "image" и ".png" только ради взятия их длины с помощью функции. Так же некоторой альтернативой могло бы быть использование функции "arg" класса QString вместо оператора "+".
Программа тестировалась настолько, на сколько это было возможно сделать в ручном режиме (компиляция, запуск, действия с заметками - проблем, кроме загадочного Timer-а, к которому мои исправления отношения не имели, не было). К сожалению, автоматические тесты в проекте найдены не были.
Естественная запись вида "переменная++" (постинкрементная) будет подводить при работе с итераторами и другими сложными типами. В случаях, когда строка с циклом всё равно изменена и будет воспринята Git-ом как новая, то почему бы не привести к виду "++переменная". К слову, даже разработчики самого Qt даже для примитивных типов используют, можно посмотреть например: http://doc.qt.io/qt-5/qstring.html. Может это осталось со старых времён, может они знают что-то ещё.
В данном случае общая стилистика С++ и раз используется этот язык, то стремление использовать его конструкции. В глобальном случая правильное использование приведения в стиле С++ может быть полезна потому, что компилятор проверяет возможность приведения одного типа к другому, в том числе, когда приведение возможно, но может привести к нежелательным последствиям.
jkollss
commented
7 years ago На мой взгляд, это был случай, когда можно поднять производительность, лишь незначительно уменьшив читаемость.
Функция вызывается сколько раз? Какой процент CPU она отъедает при суточном прогоне программы, например? Нужно вообще эту функцию оптимизировать?
А вы, кроме того, что затруднили понимание кода, так ещё и ввели какую-то магическую константу "9". Это что вообще? Как я должен догадаться что это за число? А если я поменяю "image" на "моя чертовски красивая картинка" программа что сделает?
xintrea
commented
7 years ago В общем, тут самая большая проблема в том, что все изменения сделаны одним куском. Если бы было разбито по логическим блокам, можно было бы часть принять.
А так мне проще посмотреть на изменения и внести самому только там, где они нужны.
xintrea
commented
5 years ago Изменения в пулл-реквесте были заранее несогласованы, обсуждение вылилось во вкусовщину, закрыто в связи неактуальности.
Достаточно важное исправление и оптимизация увеличения итератора в цикле, а так же оптимизация работы с циклами (проверка условий, увеличение индекса) и строками.