Conversation
sandyre
left a comment
sandyre
left a comment
There was a problem hiding this comment.
Поправимс, и будем смотреть дальше, что там по логике. Но общие замечания сразу скажу - все типы, которые являются составными (например, vector), передавать в функции по конст рефе.
| void data_release() | ||
| { | ||
| if (!_is_shallow_copy) | ||
| delete[] _data; |
There was a problem hiding this comment.
Тут тема такая. По коду много где зовется data_release, допускаю, что на одном тензоре может позваться несколько раз. Первый раз delete норм вызовется, на втором сегфолтнется. Посему, здесь надо сделать следующее:
if (!_is_shallow_copy && _data)
{
delete[] _data;
_data = nullptr
}
| using dimensions = std::vector<size_t>; | ||
|
|
||
| private: | ||
| T* _data = nullptr; |
There was a problem hiding this comment.
Давай-ка в конструкторе инитить, а то странно выглядит.
There was a problem hiding this comment.
И я бы даже заменил это на vector. Тогда и не надо никаких data_release, а просто _data.clear(). Меньше шансов облажаться где-нибудь и сделать утечку памяти, а выигрыша по скорости все равно почти нет.
|
|
||
| rhs._data = nullptr; | ||
| rhs._number_elements = 0; | ||
| rhs._dims = {}; |
There was a problem hiding this comment.
_dims.clear() выглядит пологичнее, имхо
хотя разницы в принципе нет
| return result; | ||
| } | ||
|
|
||
| Tensor operator+(const Tensor& rhs) const |
There was a problem hiding this comment.
Тут вот не понял, почему две перегрузки для сложения. Хватит перегрузки для const-reference, потому что rvalue объект неявно легко преобразуется к const-ref. В противном случае, в перегрузке что сверху всегда будет происходить лишнее копирование. И логика у них почему-то разная..
| return result; | ||
| } | ||
|
|
||
| Tensor operator*(const Tensor& rhs) const |
| return result; | ||
| } | ||
|
|
||
| Tensor operator[](int index) |
There was a problem hiding this comment.
Вот тут не понятно, я всегда буду получать копию. Тензор не модифицируем? Тогда ок.
И все индексы нужно передавать по size_t.
| Tensor operator[](int index) | ||
| { | ||
| auto new_dimensions = _dims; | ||
| new_dimensions.erase(new_dimensions.begin()); |
There was a problem hiding this comment.
Создал вектор, и потом удалил первый элемент, и таким образом вызвал перемещение всех объектов влево. Очень дорого. Есть конструктор вектора из пары итераторов, в твоем случае будет как раз
dimensions new_dims(_dims.begin() + 1, _dims.end()). Но надо бы проверять, что _dims не пустой.
| return Tensor(new_dimensions, data_offset); | ||
| } | ||
|
|
||
| const Tensor operator[](int index) const |
There was a problem hiding this comment.
В целом, перегрузка возвращающая конст объект лишена смысла. Достаточно возвращать просто объект, что с ним там дальше делать будут - не дело этого инстанса класса.
| // Linear algebra | ||
| void transpose() | ||
| { | ||
| auto tmp = _dims[0]; |
There was a problem hiding this comment.
проверка на пустоту вектора? или он гарантированно не пустой? пахнет сегфолтом
|
|
||
| private: | ||
| // sub-tensor ctor | ||
| Tensor(const dimensions& dims, T* data_start_address) |
There was a problem hiding this comment.
лучше передать const T*, чисто для читабельности.
2 participants
No description provided.