Skip to content

Фролов Кирилл#1

Open
MrPlesk2 wants to merge 8 commits intocppdevcourse:masterfrom
MrPlesk2:master
Open

Фролов Кирилл#1
MrPlesk2 wants to merge 8 commits intocppdevcourse:masterfrom
MrPlesk2:master

Conversation

@MrPlesk2
Copy link
Copy Markdown

@MrPlesk2 MrPlesk2 commented Nov 4, 2025

No description provided.

Copy link
Copy Markdown
Collaborator

@czertyaka czertyaka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Хорошая реализация, но в ней есть одна "стратегическая" ошибка — выбран не лучший способ назначать новые дескрипторы.

Comment thread stack.cpp
#define UNUSED(VAR) (void)(VAR)
#include <unordered_map>
#include <vector>
#include <stdexcept>
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Кажется, заголовок не используется. Вы везде ловите исключения через ...

Comment thread stack.cpp
const char* source_data = top_node->data.data();
char* dest_data = static_cast<char*>(data);

for (std::size_t i = 0; i < copy_size; ++i) {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Можно использовать std::memcpy

Comment thread stack.cpp
return;
}

Stack& stack = it->second;
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Посмотрите срабатывания стат. анализатора (шаг SAST в Workflow). Он вам там подсказывает, что тут можно можно сделать ссылку константной.

Comment thread stack.cpp
}
struct Node {
std::vector<char> data;
Node* next;
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Раз завернули данные в std::vector вместо сырого указателя, то тут можно было бы использовать std::optional или умный указатель.

Comment thread stack.cpp Outdated
Handle create()
{
try {
Handle handle = next_handle++;
Copy link
Copy Markdown
Collaborator

@czertyaka czertyaka Nov 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Конечно, вряд ли такое произойдет, но лучше сравнивать с std::numeric_limits<Handle>::max(), чтобы не было переполнения.

Comment thread stack.cpp Outdated
Handle create()
{
try {
Handle handle = next_handle++;
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Стратегия выделения нового идентификатора не самая лучшая. Подумайте, что произойдет в такой ситуации:

    const std::size_t max =
        std::size_t{std::numeric_limits<Handle>::max()}
        + 2;
    for (std::size_t i = 0u; i < max; ++i) {
        const Handle h = create();
        destroy(h);
    }

Такая программа одновременно держит не более одного инстанса стека. Тем не менее, ваша реализация такое использование обработает некорректно.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Такой тест, наверное, стоит добавить, но в этом случае придется менять реальный тип с int на char. Иначе тест будет идти слишком долго.

@czertyaka czertyaka assigned czertyaka and unassigned czertyaka Nov 12, 2025
Copy link
Copy Markdown
Collaborator

@czertyaka czertyaka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Посмотрите, пожалуйста, на тесты, они не прошли

Copy link
Copy Markdown
Collaborator

@czertyaka czertyaka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Задание принято. Непоправленные замечания минорные.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants