#новичкам
Большое спасибо всем участникам ревью, которые проявили активность под предыщущим постом. Не ожидал такой вовлеченности и огромного количества больших развернутых комментариев. Всем и каждому посылаю лучи благодарности!
Как всегда было сложно выбрать один комментарий, у каждого свое видение итогового решения и разные предложения. Решил отметить двух подписчиков: @Ivaneo(коммент), как наиболее продуктивного по количеству замечаний, и @monah_tuk(коммент), за более нативный формат. Давайте похлопаем им 👏👏👏.
Теперь к сути. Есть пара критических проблем в этом коде, из-за которых мы стопроцентно уйдет в неопределенное поведение, еще несколько проблем, которые влияют на корректность поведения в отдельных сценариях, остальное рюшечки, да бантики для красоты. Пойдем от критичных недостатков к косметике.
Обычно при ревью мы пытаемся максимально сохранить идею кода вымышленного автора при обсуждении решении проблем. А то можно было бы сказать "надо использовать std::vector и мозг себе не блендерить". Так не интересно)
Поехали!
🔞 Создаем массив через new[], а удаляем через delete. Это UB, нужно использовать одинаковые операторы для создания и удаления объектов и массивов.
🔞 Автор кода хочет, чтобы его класс обладал всеми специальными методами, и при этом надеется, что компилятор сможет сгенерировать их так, как надо автору. Надеяться не надо, потому что компилятор довольно тупой и выполняет поверхностное копирование и перемещение в самосгенеренных методах. Он просто копирует значение указателя в другой объект. В compareData аргументы передаются по значению, значит будет копирование. Значит после выхода из скоупа функции будет double free при разрушении второго объекта. Надо писать свои специальные методы по правилу пяти. До кучи можно использовать swap idiom.
🔞 compareData сравнивает объекты по указателю. Это неверный подход, потому что такое сравнение не предполагает сравнение самих данных. Такое сравнение будет давать в результате true только для одного и того же объекта. Нужно сравнивать данные по этим указателям, а не сами указатели.
🔞 Автор был невнимательным и забыл единичку при копировании входных данных. Таким образом в данные не попадает null-terminator, а значит не представляется возможным по одному указателю определить размер данных.
Тут вопрос на самом деле намного шире: а что мы вообще может принимать, в качестве аргументов конструктора? Туда спокойно можно передать указатель на обычный char массив, у которого нет null-terminator'а. Более того, в середине такого массива может быть валидный нулевой символ, просто как часть общего массива.
Поэтому вы правильно заметили, что будет лучше передавать вместе с указателем еще и размер данных в конструкторе. Тогда мы четко знаем размер данных и будем хранить этот размер в течение всей жизни объекта.
Плюс, если мы допускаем нулевой символ в середине, то функции типа std::str* нам не подходят, потому что ориентируются на терминатор. Лучше использовать std::copy.
Если уж очень хочется передавать строковые литералы и не передавать для них размер можно использовать такой трюк:
template <size_t N>
explicit Data(const char (&str)[N])
: Data(str, N) {
}
Второй конструктор, который будет вызываться только для чар массивов, чем на самом деле и является строковый литерал. Тогда и обычные char массивы можно передавать без размера.
Можно накрутить конструктор от std::string_view и от любых буфероподобных сущностей, но оставим это за скобками для будущих улучшений.
🔞 Использование сырых указателей попахивает С-style'ом в плюсовом коде. У нас давно появились умные указатели и если уж так хочется пользоваться чаровскими буферами(ударение не на 3-й слог!) то можно использовать std::unique_ptr<char[]>. Тогда реализация всех специальных методов становится проще, а деструктор - вообще тривиальным.