Skip to content

Konstantin Makarov#1

Open
Mako-D wants to merge 47 commits into
masterfrom
homework
Open

Konstantin Makarov#1
Mako-D wants to merge 47 commits into
masterfrom
homework

Conversation

@Mako-D

@Mako-D Mako-D commented Nov 27, 2023

Copy link
Copy Markdown
Owner

Сделал первую возможную реализацию стека. Проходит все тесты, помимо PopBadArgs (см. czertyaka#26).

@Mako-D

Mako-D commented Nov 27, 2023

Copy link
Copy Markdown
Owner Author

Коммиты Update cmake-single-platform.yml и коммиты связанные с gcc не относятся к самому ДЗ непосредственно. Это я настраивал GitHub Action для автоматической проверки тестов на платформе GitHub (раз репозиторий открытый, то им можно спокойно пользоваться)

@Mako-D Mako-D requested a review from czertyaka November 28, 2023 07:52
@Mako-D

Mako-D commented Nov 28, 2023

Copy link
Copy Markdown
Owner Author

GTest на Github Action распознал утечку памяти. Последний коммит устраняет эту проблему. Теперь проходятся все тесты, включая PopBadArgs
Задание готово для ревью х)

@Mako-D Mako-D added the enhancement New feature or request label Dec 1, 2023
@Mako-D Mako-D force-pushed the homework branch 4 times, most recently from 18ad9cc to 5e5c8b2 Compare December 2, 2023 10:37

@czertyaka czertyaka left a comment

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 CMakeLists.txt Outdated
Comment thread cstack.c Outdated
Comment thread cstack.h Outdated
Comment thread cstack.c Outdated
Comment thread cstack.c Outdated
Comment thread test.cpp Outdated
Comment thread cstack.c
Comment thread cstack.c Outdated
Comment thread cstack.c
Comment thread cstack.c Outdated

@czertyaka czertyaka left a comment

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 cstack.c
return;
}

#ifdef _MSC_VER

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.

Такие вещи обычно выносят в макрос или макро-функцию, пример:

#ifdef _MSC_VER
#define CSTACK_MEMCPY(D, DZ, S, SZ) memspy_s(D, DZ, S, SZ)
#else
#define CSTACK_MEMCPY(D, DZ, S, SZ) memcpy(D, S, SZ)
#endif

CSTACK_MEMCPY(_ptr->data, _ptr->size, data_in, size);

и пользуются дальше сколько угодно)

Copy link
Copy Markdown
Owner Author

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

Choose a reason for hiding this comment

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

Если можно избежать макро-магии и прибивания гвоздями кода к платформе/компилятору/ОС/etc — стоит избегать. Конкретно в этом случае игра свеч все таки не стоит. Но иногда этого не избежать.

@Mako-D

Mako-D commented Dec 6, 2023

Copy link
Copy Markdown
Owner Author

Задание принято)

@czertyaka

Спасибо! Можно ли спросить у вас какие-то пожелания/замечания по коду или стилю кода?

@czertyaka

Copy link
Copy Markdown
Collaborator

Задание принято)

@czertyaka

Спасибо! Можно ли спросить у вас какие-то пожелания/замечания по коду или стилю кода?

Добрый вечер! То ли не заметил ваш комментарий в ПР в прошлый раз, то ли забыл ответить)

Вообще говоря, во время ревью я старался писать все по-максимуму, за что у меня взгляд цеплялся, в том числе по coding style. Так что мне нечего особо добавить к тому, что я писал до этого.

Сейчас пересмотрел ваш код -- обратил внимание только на #pragma pack у структур. Убирать выравнивание структур в этой задаче скорее вредно, чем полезно -- экономия памяти несущественная, а вот компилятору и процессору вы немного усложинили задачу. Обращение к невыровненной памяти может оказаться медленнее, чем к выровненной.

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

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants