Repository navigation
Conversation
…rtok bug tr_array полностью переведён на emalloc/erealloc/efree. Функции записи фиксированных типов перемещены в заголовок как static inline для устранения накладных расходов на вызов. tr_array_clear теперь только сбрасывает size/position без переаллокации. Исправлен порядок CHECK_ALLOC в init; tr_array_free обнуляет поля. Исправлена ошибка в parse_domain_port_pairs: strdup перед strtok, чтобы не модифицировать исходную строку. test.php разбит на четыре .phpt-теста (settings, timer, stress, tr_array_memory) и удалён. Добавлен C-юнит-тест для parse_domain_port_pairs (8 кейсов) и tr_internal_test (32 теста для tr_array и tr_timer) с заглушкой tests/stubs/php.h, позволяющей компиляцию вне PHP. CI обновлён для запуска make test и всех C-юнит-тестов.
The header did not use any types from zend.h, but its inclusion prevented standalone compilation of unit tests outside of a PHP extension build context.
The stub provides malloc-based substitutes for emalloc/erealloc/efree and a minimal zend_resource definition, allowing tr_internal_test to compile without real PHP headers.
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (22)
WalkthroughPR adds hook tracking and serialization, revises UDP packet handling and client refresh logic, switches ChangesHooks, network serialization, and test/tooling updates
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant PHPRequest as PHP request
participant Trochilidae as trochilidae.c
participant TrHooks as tr_hooks.c
participant Zend as Zend function table
PHPRequest->>Trochilidae: RINIT
Trochilidae->>TrHooks: tr_hooks_lazy_attach()
TrHooks->>Zend: replace handler with tr_hook_bridge
PHPRequest->>Zend: call hooked function
Zend->>TrHooks: tr_hook_bridge records call_count and total_time
Trochilidae->>TrHooks: tr_hooks_serialize(&TR_G(msg))
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
trochilidae/tr_array.c (1)
53-62: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winЗащитите расчёт ёмкости от переполнения
size_t.Line 53 складывает
position + additional_size, а Line 57 удваиваетcapacityдо вызоваerealloc. При переполнении буфер может быть недовыделен, после чего следующие записи выйдут за границы.🐛 Вариант исправления
void tr_array_ensure_capacity(struct tr_array *self, const size_t additional_size) { if (additional_size == 0) return; + const size_t max_size = (size_t)-1; + if (self->position > max_size - additional_size) { + CHECK_ALLOC(NULL); + return; + } const size_t required_capacity = self->position + additional_size; - if (required_capacity < self->capacity) { + if (required_capacity <= self->capacity) { return; } - size_t new_capacity = self->capacity * 2; + size_t new_capacity = self->capacity > max_size / 2 + ? required_capacity + : self->capacity * 2; if (new_capacity < required_capacity) { new_capacity = required_capacity; } + if (new_capacity > max_size / sizeof(byte)) { + CHECK_ALLOC(NULL); + return; + } byte *new_data = (byte *)erealloc(self->data, sizeof(byte) * new_capacity);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@trochilidae/tr_array.c` around lines 53 - 62, The capacity growth logic in `tr_array.c` is vulnerable to `size_t` overflow in `ensure_capacity`/the allocation path: the `self->position + additional_size` calculation and the `self->capacity * 2` वृद्धि can wrap and lead to an undersized `erealloc`. Add explicit overflow checks before computing `required_capacity` and `new_capacity`, and handle the error case safely (e.g. fail early) so `byte *new_data` is only allocated with a verified, non-wrapped size.
🧹 Nitpick comments (12)
tests/003_stress.phpt (1)
7-10: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueИзбыточный первый вызов
trochilidae_set_hostname('test').Значение
'test'сразу перезаписывается вызовом с$hostname, и промежуточное состояние никак не проверяется — первый вызов не добавляет ценности тесту.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/003_stress.phpt` around lines 7 - 10, Remove the redundant initial call to trochilidae_set_hostname('test') in the stress test so the test only exercises the meaningful assignment via $hostname; keep the trochilidae_set_hostname sequence focused on the actual value under test and preserve the final success output.tests/002_timer.phpt (1)
26-31: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
--EXPECTF--без wildcard-шаблонов.Секция
--EXPECTF--используется без каких-либо форматных спецификаторов (%s,%dи т.п.) — весь ожидаемый вывод статичен. Логичнее использовать--EXPECT--для точного сравнения,--EXPECTF--обычно применяется, когда часть вывода непредсказуема (например, время выполнения).♻️ Предлагаемое исправление
---EXPECTF-- +--EXPECT--🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/002_timer.phpt` around lines 26 - 31, The test uses a fully static expected output under the 002_timer.phpt expectation section, so replace the current EXPECTF block with a plain EXPECT block. Update the timer test’s expectation marker only, since no wildcard placeholders are needed and the output is fixed.tests/tr_array_memory.phpt (1)
1-14: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftТест не проверяет заявленную функциональность tr_array.
Название теста и
--TEST--описание («Check tr_array functionality with new memory allocation») предполагают проверку переходаtr_arrayнаemalloc/efree/erealloc, но по факту тест лишь проверяет успешную загрузку расширения — сам комментарий в коде (строки 7-9) это подтверждает. Учитывая, что этот PR специально посвящён переходу на PHP-совместимое управление памятью вtr_array.c/tr_array.h, отсутствие реальной проверки — существенный пробел в покрытии критичного изменения.Если у расширения нет публичного PHP API для прямого воздействия на
tr_array, стоит либо добавить минимальную тестовую PHP-функцию (аналогично C unit-тестам вtests/tr_internal_test.c), либо явно пометить/переименовать тест, чтобы не вводить в заблуждение относительно покрытия.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/tr_array_memory.phpt` around lines 1 - 14, The current test only checks that the extension loads and does not verify tr_array’s new memory-allocation behavior. Update the PHPT to exercise tr_array through a real test hook or a newly exposed PHP test helper, ideally mirroring the intent of tr_array.c/tr_array.h and the existing internal test approach in tests/tr_internal_test.c. If no PHP-level access exists, either add a minimal test-only function to trigger tr_array allocation/reallocation/free paths or rename the test so it clearly reflects that it is only an extension-load smoke test.tests/tr_internal_test.c (4)
475-490: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueНеточная проверка суммарного времени в
test_tmr_accumulate.После 3×1 мс ожидается
tv_usec >= 1500. При загруженной системе сумма может быть меньше ожидаемой из-за неточностиnanosleep. Рассмотрите более низкий порог или проверку относительного соотношения.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/tr_internal_test.c` around lines 475 - 490, The assertion in test_tmr_accumulate is too strict for accumulated timing because nanosleep and scheduling can make the measured total vary. Loosen the totalExecutionTime.tv_usec expectation in test_tmr_accumulate, or replace it with a more tolerant relative check, while keeping the existing startCount/stopCount validations on TrTimer intact.
29-34: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueУтечка памяти при провале теста.
Макрос
CHECK(строка 32) выполняетreturnпри нарушении условия, не вызываяtr_array_free/tr_timer_free. При падении теста объекты остаются неосвобождёнными. Для тестового кода это допустимо, но усложняет отладку утечек.Рекомендуется использовать
goto cleanupили обёрткуwith_...:-#define CHECK(cond, msg) do { if (!(cond)) { FAIL(msg); return; } } while (0) +#define CHECK(cond, msg) do { if (!(cond)) { FAIL(msg); goto test_failed; } } while (1)С соответствующей меткой в каждой тестовой функции.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/tr_internal_test.c` around lines 29 - 34, The CHECK macro currently returns immediately on failure, bypassing cleanup and leaving tr_array_free/tr_timer_free allocations unfreed in test functions. Update the test helpers around CHECK/FAIL so failures jump to a cleanup path instead of returning directly, and add a cleanup label in each affected test function to release any allocated resources before exiting.
496-535: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueОтсутствие освобождения памяти при падении теста в
main().Если какой-либо тест падает,
main()возвращает 1, но не освобождает ресурсы уже созданных объектов. Это приемлемо для unit-test процесса, но при массовых падениях усложняет анализ Valgrind/ASan.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/tr_internal_test.c` around lines 496 - 535, The main test runner currently returns immediately on failure without cleaning up resources from earlier tests; update main() to perform a shared teardown path before exiting, using the existing test helpers and any free/reset routines involved in tr_array and tr_timer setup. Make the exit logic go through a single cleanup section so all allocated objects are released whether tests pass or fail, while keeping the current passed/tests result reporting intact.
462-473: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueШирокий диапазон проверки
tv_secв длительном тесте.
test_tmr_long_runningпроверяетtv_sec <= 2послеnanosleep(1 сек). При загруженной системе задержка может превысить 2 секунды, что приведёт к flaky-тесту. Рассмотрите более мягкую верхнюю границу (например,<= 5).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/tr_internal_test.c` around lines 462 - 473, The long-running timer test in test_tmr_long_running uses a too-strict upper bound on t->totalExecutionTime.tv_sec after nanosleep, which can make the test flaky on busy systems. Relax the sanity check in test_tmr_long_running by increasing the allowed upper limit to a more tolerant value while keeping the lower bound that confirms at least one second elapsed, and leave the rest of the TrTimer flow unchanged.tests/unit_test.c (4)
1-136: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winОтсутствие теста на
NULL-вход.Нет теста для
parse_domain_port_pairs(NULL, &numPairs). Согласно реализации,strdup(NULL)вызовет UB (segfault). Стоит добавить защиту в продакшен-код или хотя бы задокументировать поведение.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit_test.c` around lines 1 - 136, Add coverage for the NULL input path in parse_domain_port_pairs, since the current tests in test_empty_string and the other parse_domain_port_pairs cases do not exercise parse_domain_port_pairs(NULL, &numPairs). Either update parse_domain_port_pairs to safely reject a NULL domain string and set numPairs to 0 with a NULL/empty result, or explicitly define and document the expected failure behavior and add a matching test to lock it in.
122-136: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueНеточное сообщение в
test_empty_string.Сообщение об ошибке
"expected 1 pairs"грамматически неверно (должно быть"expected 0 pairs"или"expected 1 pair"). Это мелочь, но указывает на путаницу в ожидаемом поведении.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit_test.c` around lines 122 - 136, The failure message in test_empty_string is inconsistent with the expected count; update the assertion text so it matches the actual expectation and uses correct singular/plural wording. Locate the message in the unit test helpers around test_empty_string and change it to either “expected 0 pairs” if the test expects no results, or “expected 1 pair” if it expects one result, so the wording and behavior align.
55-64: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low value
malloc(0)может вернутьNULL.Для пустой строки
strlen(input_dup)равно 1, поэтомуmalloc(1 * sizeof(DomainPortEntry))выделяет ненулевой размер. Однако если бы строка была пустой сstrlen = 1,malloc(0)возвращает NULL на некоторых реализациях. ПроверкаASSERT(pairs != NULL, ...)была бы хрупкой. В текущем коде это не критично, но стоит учитывать.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit_test.c` around lines 55 - 64, In test_empty_string, avoid relying on a non-NULL return from parse_domain_port_pairs for the empty-input case because malloc(0) semantics can vary; update the test to focus on numPairs being 0, or adjust parse_domain_port_pairs to always return a stable allocation when input is empty. Use parse_domain_port_pairs and test_empty_string to locate the check and keep the assertion aligned with the intended behavior.
10-14: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueУтечка памяти при провале теста.
Макрос
ASSERT(строка 13) выполняетreturnбез освобожденияpairs. При падении теста память не освобождается. Рекомендуется добавитьcleanup:метки или использоватьgoto.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit_test.c` around lines 10 - 14, The ASSERT macro currently returns immediately on failure without freeing pairs, causing a memory leak when a test aborts. Update the test flow around ASSERT so failures jump to a cleanup path instead of returning directly, and ensure the cleanup logic frees pairs before exiting the test function. Use the existing ASSERT macro and the test case function’s cleanup section (or add a cleanup label with goto) to centralize resource release.tests/.gitignore (1)
1-4: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueПроверьте, что
!*.cне допускает нежелательных файлов.Паттерн
!*.cразрешает все файлы с расширением.cв директорииtests/. Если в будущем появятся вспомогательные.cфайлы (например, сгенерированные), они будут отслеживаться. Рассмотрите более конкретные исключения:!unit_test.c,!tr_internal_test.c, или используйте поддиректориюtests/src/*.c.Также уточните
!stubs— возможно, стоит использовать!stubs/для явного указания директории.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/.gitignore` around lines 1 - 4, The tests/.gitignore rules are too broad because !*.c will unignore every C file under tests/, which can accidentally admit future generated or helper sources. Narrow the ignore exceptions in the relevant ignore block by replacing the generic .c pattern with explicit allowed test filenames or a dedicated subdirectory, and make the stubs exception directory-specific by using the symbols !*.c and !stubs in a more precise form such as file-specific or folder-only matching.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/build.yml:
- Around line 30-31: The unit test compile step hardcodes PHP include
directories, which can break across the PHP versions installed by setup-php.
Update the compile command in the build workflow to use include flags from
php-config --includes instead of the /usr/include/php paths, matching the
approach used in CMakeLists.txt and keeping tests building across the version
matrix.
In `@tests/stubs/php.h`:
- Around line 1-18: The PHP stub is missing the CHECK_ALLOC macro used by
tr_array initialization after emalloc, so tests that include tr_array.c will
fail to compile. Add a minimal CHECK_ALLOC definition in php.h alongside the
existing memory wrappers so calls from the tr_array code resolve correctly; keep
the fix focused on the stub definitions and use the existing emalloc/efree style
symbols to locate the right place.
In `@tests/unit_test.c`:
- Around line 55-64: `test_empty_string` has the wrong expectation for the
`parse_domain_port_pairs` contract: for an empty input, `numPairs` is left as 1
rather than 0. Update the assertion in this test to match the actual behavior of
`parse_domain_port_pairs` in `trochilidae/tr_network.c` and keep the rest of the
`test_empty_string` setup unchanged.
In `@trochilidae/tr_array.h`:
- Around line 66-89: Update the write helpers in tr_array so size tracks the new
end position instead of being incremented blindly. In tr_array_write_byte,
tr_array_write_short, tr_array_write_word, and tr_array_write_long, keep
advancing position by the written width, but set size based on the resulting
position when it exceeds the previous size. This ensures writes after
tr_array_set_position() do not overcount or undercount the buffer length.
In `@trochilidae/tr_network.c`:
- Around line 68-76: In parse_domain_port_pairs, guard the strdup(input) result
before using it, because input_dup may be NULL and strlen on it will crash. Also
replace the current malloc sizing based on strlen(input_dup) with a checked
allocation size that uses the actual number of DomainPortEntry elements needed
and verifies there is no overflow before allocating pairs. Keep the existing
error path and freeing logic in parse_domain_port_pairs, but make sure both the
strdup and allocation failures are handled safely before any dereference or size
calculation.
---
Outside diff comments:
In `@trochilidae/tr_array.c`:
- Around line 53-62: The capacity growth logic in `tr_array.c` is vulnerable to
`size_t` overflow in `ensure_capacity`/the allocation path: the `self->position
+ additional_size` calculation and the `self->capacity * 2` वृद्धि can wrap and
lead to an undersized `erealloc`. Add explicit overflow checks before computing
`required_capacity` and `new_capacity`, and handle the error case safely (e.g.
fail early) so `byte *new_data` is only allocated with a verified, non-wrapped
size.
---
Nitpick comments:
In `@tests/.gitignore`:
- Around line 1-4: The tests/.gitignore rules are too broad because !*.c will
unignore every C file under tests/, which can accidentally admit future
generated or helper sources. Narrow the ignore exceptions in the relevant ignore
block by replacing the generic .c pattern with explicit allowed test filenames
or a dedicated subdirectory, and make the stubs exception directory-specific by
using the symbols !*.c and !stubs in a more precise form such as file-specific
or folder-only matching.
In `@tests/002_timer.phpt`:
- Around line 26-31: The test uses a fully static expected output under the
002_timer.phpt expectation section, so replace the current EXPECTF block with a
plain EXPECT block. Update the timer test’s expectation marker only, since no
wildcard placeholders are needed and the output is fixed.
In `@tests/003_stress.phpt`:
- Around line 7-10: Remove the redundant initial call to
trochilidae_set_hostname('test') in the stress test so the test only exercises
the meaningful assignment via $hostname; keep the trochilidae_set_hostname
sequence focused on the actual value under test and preserve the final success
output.
In `@tests/tr_array_memory.phpt`:
- Around line 1-14: The current test only checks that the extension loads and
does not verify tr_array’s new memory-allocation behavior. Update the PHPT to
exercise tr_array through a real test hook or a newly exposed PHP test helper,
ideally mirroring the intent of tr_array.c/tr_array.h and the existing internal
test approach in tests/tr_internal_test.c. If no PHP-level access exists, either
add a minimal test-only function to trigger tr_array
allocation/reallocation/free paths or rename the test so it clearly reflects
that it is only an extension-load smoke test.
In `@tests/tr_internal_test.c`:
- Around line 475-490: The assertion in test_tmr_accumulate is too strict for
accumulated timing because nanosleep and scheduling can make the measured total
vary. Loosen the totalExecutionTime.tv_usec expectation in test_tmr_accumulate,
or replace it with a more tolerant relative check, while keeping the existing
startCount/stopCount validations on TrTimer intact.
- Around line 29-34: The CHECK macro currently returns immediately on failure,
bypassing cleanup and leaving tr_array_free/tr_timer_free allocations unfreed in
test functions. Update the test helpers around CHECK/FAIL so failures jump to a
cleanup path instead of returning directly, and add a cleanup label in each
affected test function to release any allocated resources before exiting.
- Around line 496-535: The main test runner currently returns immediately on
failure without cleaning up resources from earlier tests; update main() to
perform a shared teardown path before exiting, using the existing test helpers
and any free/reset routines involved in tr_array and tr_timer setup. Make the
exit logic go through a single cleanup section so all allocated objects are
released whether tests pass or fail, while keeping the current passed/tests
result reporting intact.
- Around line 462-473: The long-running timer test in test_tmr_long_running uses
a too-strict upper bound on t->totalExecutionTime.tv_sec after nanosleep, which
can make the test flaky on busy systems. Relax the sanity check in
test_tmr_long_running by increasing the allowed upper limit to a more tolerant
value while keeping the lower bound that confirms at least one second elapsed,
and leave the rest of the TrTimer flow unchanged.
In `@tests/unit_test.c`:
- Around line 1-136: Add coverage for the NULL input path in
parse_domain_port_pairs, since the current tests in test_empty_string and the
other parse_domain_port_pairs cases do not exercise
parse_domain_port_pairs(NULL, &numPairs). Either update parse_domain_port_pairs
to safely reject a NULL domain string and set numPairs to 0 with a NULL/empty
result, or explicitly define and document the expected failure behavior and add
a matching test to lock it in.
- Around line 122-136: The failure message in test_empty_string is inconsistent
with the expected count; update the assertion text so it matches the actual
expectation and uses correct singular/plural wording. Locate the message in the
unit test helpers around test_empty_string and change it to either “expected 0
pairs” if the test expects no results, or “expected 1 pair” if it expects one
result, so the wording and behavior align.
- Around line 55-64: In test_empty_string, avoid relying on a non-NULL return
from parse_domain_port_pairs for the empty-input case because malloc(0)
semantics can vary; update the test to focus on numPairs being 0, or adjust
parse_domain_port_pairs to always return a stable allocation when input is
empty. Use parse_domain_port_pairs and test_empty_string to locate the check and
keep the assertion aligned with the intended behavior.
- Around line 10-14: The ASSERT macro currently returns immediately on failure
without freeing pairs, causing a memory leak when a test aborts. Update the test
flow around ASSERT so failures jump to a cleanup path instead of returning
directly, and ensure the cleanup logic frees pairs before exiting the test
function. Use the existing ASSERT macro and the test case function’s cleanup
section (or add a cleanup label with goto) to centralize resource release.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: a0415c77-f1d3-4381-af15-c8c74ac26e1b
📒 Files selected for processing (16)
.github/workflows/build.yml.gitignore.opencode/instructions.mdtest.phptests/.gitignoretests/001_settings.phpttests/002_timer.phpttests/003_stress.phpttests/stubs/php.htests/tr_array_memory.phpttests/tr_internal_test.ctests/unit_test.ctrochilidae/tr_array.ctrochilidae/tr_array.htrochilidae/tr_network.ctrochilidae/tr_network.h
💤 Files with no reviewable changes (2)
- test.php
- trochilidae/tr_network.h
Comment on lines
+30
to
+31
| - name: Compile unit tests | ||
| run: gcc -I/usr/include/php -I/usr/include/php/main -I/usr/include/php/Zend -I/usr/include/php/TSRM -I/usr/include/php/ext -I. -DHAVE_CONFIG_H -D_GNU_SOURCE -g -O0 tests/unit_test.c trochilidae/tr_network.c trochilidae/utils.c -o tests/unit_test |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Проверка корректности путей к PHP-заголовкам
php-config --include-dir
php-config --includesRepository: xMlex/php_trochilidae
Length of output: 251
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== workflow outline =="
ast-grep outline .github/workflows/build.yml || true
echo "== relevant workflow lines =="
nl -ba .github/workflows/build.yml | sed -n '1,220p'
echo "== search for php setup and matrix references =="
rg -n --hidden --glob '.github/workflows/*.yml' --glob '.github/workflows/*.yaml' 'php-config|phpize|setup-php|include-dir|matrix|ubuntu-latest|PHP' .github/workflows || true
echo "== search repo for php-config usage =="
rg -n --hidden 'php-config|phpize|setup-php|/usr/include/php|/usr/include/php/' . || trueRepository: xMlex/php_trochilidae
Length of output: 294
🏁 Script executed:
#!/bin/bash
set -euo pipefail
nl -ba .github/workflows/build.yml | sed -n '1,120p'Repository: xMlex/php_trochilidae
Length of output: 199
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== build.yml lines =="
sed -n '1,220p' .github/workflows/build.yml
echo "== workflow file size =="
wc -l .github/workflows/build.yml
echo "== php-related searches =="
grep -RIn --exclude-dir=.git -E 'php-config|phpize|setup-php|/usr/include/php|ubuntu-latest|matrix' .github/workflows . || trueRepository: xMlex/php_trochilidae
Length of output: 4213
Заменить хардкод путей на php-config --includes
setup-php поднимает разные версии PHP из матрицы, а /usr/include/php/... зависит от API-версии и может отсутствовать. Для этого шага лучше брать include-флаги из php-config (как в CMakeLists.txt), иначе сборка unit-теста сломается на части версий PHP.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/build.yml around lines 30 - 31, The unit test compile step
hardcodes PHP include directories, which can break across the PHP versions
installed by setup-php. Update the compile command in the build workflow to use
include flags from php-config --includes instead of the /usr/include/php paths,
matching the approach used in CMakeLists.txt and keeping tests building across
the version matrix.
Comment on lines
+1
to
+18
| #ifndef STUB_PHP_H | ||
| #define STUB_PHP_H | ||
|
|
||
| #define _GNU_SOURCE | ||
| #include <stdlib.h> | ||
| #include <string.h> | ||
| #include <stdio.h> | ||
| #include <sys/time.h> | ||
|
|
||
| #define emalloc(s) malloc(s) | ||
| #define erealloc(p, s) realloc(p, s) | ||
| #define efree(p) free(p) | ||
| #define estrdup(s) strdup(s) | ||
|
|
||
| typedef unsigned char zend_bool; | ||
| typedef struct { void *ptr; } zend_resource; | ||
|
|
||
| #endif |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Отсутствует макрос CHECK_ALLOC.
В trochilidae/tr_array.c (строки 9-22) используется CHECK_ALLOC(self->data) после emalloc. Этот макрос не определён в заглушке, что приведёт к ошибке компиляции при включении tr_array.c в тесты.
Добавьте минимальную реализацию, например:
`#define` estrdup(s) strdup(s)
+#define CHECK_ALLOC(p) do { if (!(p)) { fprintf(stderr, "alloc failed\n"); exit(1); } } while(1)
+
typedef unsigned char zend_bool;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/stubs/php.h` around lines 1 - 18, The PHP stub is missing the
CHECK_ALLOC macro used by tr_array initialization after emalloc, so tests that
include tr_array.c will fail to compile. Add a minimal CHECK_ALLOC definition in
php.h alongside the existing memory wrappers so calls from the tr_array code
resolve correctly; keep the fix focused on the stub definitions and use the
existing emalloc/efree style symbols to locate the right place.
- tests/install_test.sh verifies make install (checks .so exists, runs PHP smoke test) - Dockerfile: remove 'exit 1', fix build (make -j, no 'make clean' before install), add install_test verification - tests/.gitignore: allow *.sh scripts
Owner
Author
|
/oc summarize |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
Dockerfile (1)
123-131: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueBuild-зависимости для trochilidae не удаляются после сборки.
В отличие от других этапов (imap, gd, amqp/redis/ssh2), пакеты
build-base autoconf unzip git curlостаются в финальном образе, увеличивая его размер.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Dockerfile` around lines 123 - 131, The trochilidae build stage leaves temporary build packages in the final image, unlike the other extension steps. Update the Dockerfile around the trochilidae build sequence to install the needed build dependencies only for the build, then remove them after `make install`/`docker-php-ext-enable`/`tests/install_test.sh`, using the same cleanup pattern already used in the imap, gd, amqp, redis, and ssh2 steps.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@README.md`:
- Around line 59-68: The README example for trochilidae_timer_get_info is
incorrect and should match the extension’s actual API. Update the Quick start
and PHP Functions docs to show that trochilidae_timer_get_info takes no
arguments and returns a nested "timers" array with camelCase fields like
startCount, stopCount, totalExecutionTime, and lastExecutionTime, rather than a
flat array with execution_time/total/start_count/stop_count. Also update the
related usage in the install test to reflect the real return structure and
signature.
In `@tests/install_test.sh`:
- Around line 33-46: The smoke test is calling trochilidae_timer_get_info with a
timer name and asserting on start_count/stop_count fields that do not exist.
Update the test around trochilidae_timer_get_info and the smoke timer checks to
use the API as implemented: call it without a timer argument, then verify the
returned timers collection and its startCount/stopCount values for the smoke
entry. Keep the existing trochilidae_timer_start and trochilidae_timer_stop
setup, but align the assertions with the actual response shape so the install
test no longer fails.
---
Nitpick comments:
In `@Dockerfile`:
- Around line 123-131: The trochilidae build stage leaves temporary build
packages in the final image, unlike the other extension steps. Update the
Dockerfile around the trochilidae build sequence to install the needed build
dependencies only for the build, then remove them after `make
install`/`docker-php-ext-enable`/`tests/install_test.sh`, using the same cleanup
pattern already used in the imap, gd, amqp, redis, and ssh2 steps.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 8d717750-0ad7-4f2c-88e1-6f0d09fffddf
📒 Files selected for processing (8)
.github/workflows/opencode.yml.gitignore.travis.ymlDockerfileREADME.mdtests/.gitignoretests/install_test.shtrochilidae/tr_array.c
💤 Files with no reviewable changes (1)
- .travis.yml
✅ Files skipped from review due to trivial changes (3)
- .github/workflows/opencode.yml
- tests/.gitignore
- .gitignore
🚧 Files skipped from review as they are similar to previous changes (1)
- trochilidae/tr_array.c
- Function takes no arguments and returns nested array with camelCase keys - README quick start and function table corrected - install_test.sh assertions fixed to match real API
- send_data(): free strdup fallbacks for request_domain/request_uri
- send_data(): remove useless strdup("argv") leak
- update_server_list(): clean old collectors beyond new limit, NULL host after free
- tr_client_destroy(): socketFd >= 0 check (was > 0), NULL host after free
- find_domain_resolve_cache_lru_entry_index(): guard for empty cache
- parse_domain_port_pairs(): max_pairs size_t vs int signedness
- tests/udp_test_server.php: PHP-based UDP server that receives chunked packets, reassembles, and parses the binary protocol - Integrated into install_test.sh as step 4 (UDP integration test) - tests/.gitignore: allow *.php files
- INI: trochilidae.hook_list (comma-separated, supports function, Class->method, Class::method, Class.method) and trochilidae.debug (E_WARNING on miss) - tr_hooks_lazy_attach: parses hook_list once at first RINIT, replaces zend_function handlers with a generic bridge that collects call_count + total_time - tr_hooks_reset: zeroes counters per request - tr_hooks_serialize: appends hook metrics to UDP packet after timers section - Generic tr_hook_bridge handles all hooks via execute_data->func pointer comparison - Duplicate detection, skip uninstalled modules, debug warnings - utils.c: fix prng_seeded scope (static inside #else block)
- New INI with PHP_INI_ALL, validated in update_server_list after tr_client_init - Values < 22 or > 65507 revert to MAX_CHUNK_SIZE
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit