Сомов Ярослав #9
Conversation
|
@maksimus-sergeyev, @KirillovAndrey, @VarvaraSharutina можете проверять |
include/stack.h
Outdated
| else throw std::runtime_error("Trying to pop from empty stack"); | ||
| } | ||
|
|
||
| void clear() { while (!isEmpty()) pop(); } |
There was a problem hiding this comment.
зачем тут целый цикл? просто sz = 0 не проще?
в зависимости от смысла функции, можно при случае не только sz = 0, но и память освободить частично либо полностью
| TStack& operator=(TStack&& s) noexcept | ||
| { | ||
| TStack tmp(s); | ||
| swap(*this, s); | ||
| return *this; | ||
| } |
There was a problem hiding this comment.
утечка памяти. должно быть что-то вроде
TStack& operator=(TStack&& s) noexcept
{
if (this == &s) return *this;
delete[] data;
data = s.data;
sz = s.sz;
cap = s.cap;
s.cap = s.sz = 0;
s.data = nullptr;
return *this;
}
upd: здесь, кстати, тоже не правда, получается. утечки нет, естественно. при условии, что компилятор догадается вызвать перемещающий конструктор, а по идее он должен догадаться, здесь всё приемлемо было.
There was a problem hiding this comment.
здесь, кстати, тоже не правда, получается. утечки нет, естественно. при условии, что компилятор догадается вызвать перемещающий конструктор, а по идее он должен догадаться, здесь всё приемлемо было.
There was a problem hiding this comment.
держу в курсе, я ещё забыл проверку на самоприсваивание
| ~TStack() | ||
| { | ||
| sz = 0; | ||
| cap = 0; | ||
| delete[] data; | ||
| } |
There was a problem hiding this comment.
желательно ещё data = nullptr;
поскольку, например, деструктор можно вызвать принудительно, то бишь вручную,
например так
PtrClass * ptr = new PtrClass();
ptr->~PtrClass();
то при повторном вызове деструктора, не положив data := nullptr,
на строке
delede[] data;
вылетит ошибка о попытке повторного освобождения памяти (upd: в зависимости от стандарта и компилятора может даже и не ошибка, а неопределенное поведение будет).
А, если всё-таки data = nullptr, то delete от nullptr'а, согласно стандарту, должен отработать корректно, то есть никак:
"The value of the first argument supplied to a deallocation function may be a null pointer value; if so, and if the deallocation function is one supplied in the standard library, the call has no effect."
| TStack(const TStack& s) : sz(s.sz), cap(s.cap) | ||
| { | ||
| data = new T[cap]; | ||
| std::copy(s.data, s.data + cap, data); | ||
| } |
There was a problem hiding this comment.
можно копировать не весь стек вплоть до cap, а только до sz
|
ещё я должен осудить комментарии на русском, поскольку Дмитриевич говорил, всё на английском писать |
|
если мне не изменяет память, Дмитриевич хотел возможность в программе |
|
в остальном: |
¯\(ツ)/¯
фича уже есть, нужно вводить конструкции вида "название переменной" = значение |
Не, речь не о том. На этом примере: Ну и т.д. А Дмитриевич хотел видеть, что-то типа: Т.е. если я правильно помню, выражение должно вводиться одноразово, а потом должна быть возможность посчитать его для разных значений переменных без повторного ввода выражения |
|
ещё можно придраться, что деление на 0 считается = inf, хотя скорее должно быть не определено и выдавать ошибку |
как бы это странно не звучало, но по IEEE 754 при делении положительных чисел возвращается inf, а при делении отрицательных -inf |
стандарт стандартом, но тебе же никто не мешает при делении проверить второй аргумент на равность нулю и, скажем, кинуть исключение. с точки зрения математики, логичнее так сделать, я считаю |
|
|
||
| void clear() | ||
| { | ||
| TStack s; | ||
| swap(*this, s); | ||
| } | ||
|
|
There was a problem hiding this comment.
почему? при выходе из функции будет вызван деструктор, который уничтожит TStack s (у него к этому моменту будет указатель на старые данные)
|
|
||
| TStack& operator=(const TStack& s) | ||
| { | ||
| if (this == &s) | ||
| return *this; | ||
| TStack tmp(s); | ||
| swap(*this, tmp); | ||
| return *this; | ||
| } | ||
|
|
There was a problem hiding this comment.
тоже будет вызван деструктор у TStack tmp при выходе
| TStack& operator=(TStack&& s) noexcept | ||
| { | ||
| delete[] data; | ||
| sz = 0; | ||
| cap = 0; | ||
| data = nullptr; | ||
| swap(*this, s); | ||
| return *this; | ||
| } |
There was a problem hiding this comment.
upd: ещё проверка на самоприсваивание должна быть
There was a problem hiding this comment.
спорный на самом деле вопрос, потому что нарочно сделать самоприсваивание по rvalue невозможно (rvalue - временный объект, как можно превратить уже существующий объект во временный?). можно конечно напридумывать конструкций вроде st=std::move(st) чтобы сделать самоприсваивание, но это уже что-то на уровне стрельбы из ружья по своим ногам.
к тому же мне кажется в коде уже поздно что-то менять
There was a problem hiding this comment.
вообще не спорный. в обычном операторе= ты ж делаешь проверку от a = a, тоже ведь выстрел в ногу; почему здесь не сделать от st=std::move(st)? ценой одной проверки исключаешь стрельбу по ногам. на счет поздно: согласен, но, ты Дмитриевича вроде до сих пор не тегнул, хотя уже давно пора
There was a problem hiding this comment.
в таком случае лучше assert делать (если адрес у rvalue каким-то образом оказался равен адресу объекта, то это явно какой-то баг), ну и я клоню к тому, что такие ситуации естественным образом не возникают.
There was a problem hiding this comment.
даже ассерт лучше, чем ничего. но, я всё же считаю, что надо проверять и возвращать *this. и всякие разработчики stl'ей, в частности, мелкософт, предпочитают таки делать так же.(github.com/microsoft/STL/blob/main/stl/inc/vector строка 740)
|
@ValentinV95, код готов к проверке |
| TStack<int> s; | ||
| s.push(42); | ||
| ASSERT_FALSE(s.isEmpty()); | ||
| } |
There was a problem hiding this comment.
Вроде всё верно (извиняюсь, за то что долго не давала никакую реакцию, так как я не нашла ошибок, не считала нужным об этом писать)
ValentinV95
left a comment
ValentinV95
left a comment
There was a problem hiding this comment.
Лабораторная и отчет приняты.
5 participants
No description provided.