[CI] Restore the per-function Doxygen check - #688
Conversation
|
[Automated review by a separate Claude agent, relayed to this PR] Scope: read nnstreamer/nnstreamer#4908 and the full review history of nnstreamer/nnstreamer#4914 ( Does it solve the stated problemYes. Backlog = 0 claim — verified independentlyI could not run
I did not find a counterexample. The claim holds as far as I can verify it. Self-test / CI wiring
Positive note on the port, not an issue: in nnstreamer's should-fix (not blocking this PR — outside its diff, but answers a question this review was asked to check)
Informational / no PR action needed
Size / scopeProportionate and minimal: 3 files, all under Architecture / API docsNone needed. CI-script-only change; this repo has no CI status at review time
VerdictMergeable on content. The restored checker and its self-test are a faithful, verified port of the version that survived 4 rounds of review upstream, the "zero backlog" claim checks out under independent verification (including the highest-volume risk category, the 438 GTest macros), and the PR is minimal and correctly scoped. The one should-fix (branch protection not actually requiring "Static checks") is real and worth a maintainer follow-up, but it's a repository setting outside this PR's diff, not a reason to hold this PR. |
"gievn" for "given" in the brief of the offloading URI helper. The typos job flags it on every PR that runs the spell check, including ones that do not touch this file (#688). Signed-off-by: MyungJoo Ham <myungjoo.ham@samsung.com> Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
myungjoo-bot
left a comment
There was a problem hiding this comment.
Automated review (transcribed from an AI review agent's report; please verify before acting).
Summary: The PR fixes the top-level local that emptied the ctags kind list (making the per-function Doxygen check pass vacuously), defaults report_path, resets brief per file, adds /**< trailing-comment and block-comment tracking, and adds an 18-fixture self-test wired into static.check.yml (trigger pull_request, no secrets). Verified locally under Exuberant Ctags 5.9 (the flavor CI installs): test_doxygen_tag.sh 18/18 PASS, exit 0 (matching the CI run). doxygen-tag.sh was also run on 27 additional probes (undocumented definition, gtest TEST(), #if 0, static inline, multi-line, K&R, C++ out-of-line methods with a lambda, templates, macro-declared functions, prototypes in .h vs .c, extern "C", brace-in-string) with the expected results, and on all 48 C/C++ files at the PR head: exit 0, 0 errors, 1166 function/prototype tags inspected. git merge-tree against current main is clean. The failing "Spell Check with Typos" job is unrelated (no source files touched). Approving; the items below are non-blocking and mostly pre-existing.
- [Low]
/**<inside a function body counts as documentation —.github/workflows/static.check.scripts/doxygen-tag.sh:54(has_trailing_doc): the/**<test runs before the brace-depth update. Reproduced:int foo (int x) { int y; /**< local */ return x + y; }with no leading comment passes. Fix: updatedepthfirst and accept only whendepth <= 0. - [Low] One-line
/** @brief foo */ int foo (int x);is flagged —:136vs:150: the function check runs before the current line's@briefis registered (pre-existing ordering; no instance in the tree). Also at:137the unquotedecho $line | cut ...glob-expands/**, so the message printed/bin function needs @brief tag. Fix: test@brief|@copydocbefore line 136, and use"${line%% *}"in the message. - [Low] Preprocessor conditional between doc comment and definition resets the pending brief —
:155(pre-existing; no instance in the tree). Exclude#if/#ifdef/#ifndef/#else/#elif/#endiffrom the reset. - [Low] Non-ASCII files are skipped entirely —
:70file $file | grep "ASCII text":c/include/ml-api-common.h(contains®) is skipped by every rule. It passes with 0 errors once ASCII-ified, so widening the gate has no backlog. Fix:grep -E "ASCII text|UTF-8 (Unicode )?text". - [Low] Self-test gaps —
test_doxygen_tag.sh:83-249: no.ccfixture (gtestTEST()/ out-of-lineC::m) and no#if 0fixture, although both behave correctly under Exuberant Ctags; two fixtures would pin them.:33checkscommand -v ctagsbut not the flavor; actags --version | grep -q Exuberantguard with a clear message would help local runs. - [Info] Workflow wiring (
static.check.yml:86-102) is correct: runs onpull_request, handles the merge-ref/old-base mismatch, catches deletions via--diff-filter=D --no-renames, and fails the step on non-zero exit.
No back-door or suspicious behavior found: trigger is pull_request (not pull_request_target), no secrets or write token, no eval, and the self-test writes only under mktemp -d.
doxygen-tag.sh declared `local function_check_flag="f+p"` at script top level. Bash rejects `local` outside a function, the script has no `set -e`, so ctags ran with an empty kind list and the per-function @brief check never fired. The script is byte-identical to the copy in nnstreamer/nnstreamer, where the same defect was found and fixed in nnstreamer/nnstreamer#4914 (issue nnstreamer/nnstreamer#4908); this takes that checker and its self-test verbatim. - Drop the `local`, default report_path to /dev/null (it was unset and produced an "ambiguous redirect" per file), and reset the brief state per file. - Check function definitions everywhere but prototypes only in headers; a static forward declaration in a .c is documented at its definition. - Recognise the trailing `/**< ... */` form, and track block comments so a '*' at the start of a wrapped parameter line is not taken for a comment continuation. - Add test_doxygen_tag.sh (18 generated fixtures) and a workflow step for it with the same merge-ref guard the nnstreamer copy uses. The tree is already clean under the corrected scope: 48 C files, zero findings, so the check turns on with nothing to clear. Signed-off-by: MyungJoo Ham <myungjoo.ham@samsung.com> Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
0800caa to
1222a6b
Compare
myungjoo-bot
left a comment
There was a problem hiding this comment.
Automated re-review of head 1222a6ba79 after the rebase onto main (transcribed from an AI review agent's report; please verify before acting).
Summary: The PR content is unchanged from the previously approved head; only the base moved to main 8952123. git merge-tree is clean, GitHub reports MERGEABLE (blocked only on required review), and all 10 checks pass on the current head, including the previously red "Spell Check with Typos" (fixed on main by #689) and Static checks whose log shows the new self-test 18/18 PASS under CI's exuberant-ctags. Real ctags was not available locally this time, so the scripts were exercised against a ctags -x emulator: self-test 18/18 in both Git Bash and WSL, a 21-fixture probe suite behaves as designed, and a whole-tree run over all 48 C/C++ files (1110 function/prototype tags) exits 0 with zero errors. The five Low findings from the previous review were each re-verified and still hold; a 4-line patch addressing (1)-(4) below was also verified to keep the self-test, probes, and whole tree green (including c/include/ml-api-common.h, which the widened text gate would newly check). Approval stands; the items below remain non-blocking follow-ups.
- [Low]
/**<inside a function body is accepted as documentation —doxygen-tag.sh:54(has_trailing_doc): the test runs before the brace-depth update at:55-57. Fix:[[ $depth -le 0 && $l == *"/**<"* ]] && return 0after the depth update. - [Low] One-line
/** @brief foo */ int foo (int x);is flagged; error message glob-expands —:136-137vs:150. Fix: add&& ! $line =~ @(brief|copydoc)to the condition at:136(do not move the@briefblock above the check, or the;reset would flag every prototype), and print the name via parameter expansion instead of the unquotedecho $line | cut. - [Low] Preprocessor conditional between doc comment and definition resets the pending brief —
:155. Fix: exclude^[[:space:]]*#[[:space:]]*(if|ifdef|ifndef|elif|else|endif)from the reset (#defineshould still reset). - [Low] Non-ASCII sources are skipped entirely —
:70grep "ASCII text";c/include/ml-api-common.hisUTF-8 textand is never checked. Fix:grep -E "ASCII text|UTF-8"; the tree still passes with that gate. - [Low] Self-test gaps —
test_doxygen_tag.sh:83-249: no.cc/ gtestTEST()/ out-of-lineC::mfixture and no#if 0fixture;:33checks onlycommand -v ctagswithout a flavor guard. Suggest adding fixtures for items 1-4 when they are fixed. - [Low] Block-comment tracker can be stuck open by
/*inside a//comment or string literal —:163. Only a false negative is possible and no instance exists in the tree; optional: skip lines starting with//before the/*test.
No back-door or suspicious behavior found: trigger is pull_request (not pull_request_target), no secrets or write token, no eval; the only ${{ }} interpolations are head.sha and commits, matching the pre-existing pattern.
Port of nnstreamer/nnstreamer#4914 (issue nnstreamer/nnstreamer#4908).
What was wrong
.github/workflows/static.check.scripts/doxygen-tag.shhere is byte-identical to the nnstreamer copy:local function_check_flag="f+p"at script top level, which bash rejects, and noset -e, soctags -x --c-kinds=ran with an empty kind list and the per-function@briefcheck has passed vacuously since the Actions import.$report_pathwas never set (an "ambiguous redirect" per file on stderr) and$briefwas not reset between files.What this PR does
Takes the corrected checker and its self-test from nnstreamer/nnstreamer#4914 verbatim, and adds the self-test step to
static.check.ymlwith the same merge-ref guard:/**< ... */form is recognised (declaration line, a line opening with/**<right after it, or after an inline body);*at the start of a wrapped parameter line is not taken for a comment continuation;report_pathdefaults to/dev/null,briefresets per file;test_doxygen_tag.shgenerates 18 fixtures into a temp dir; all 18 fail against the checker onmainand pass here.Backlog
None. Under the corrected scope the tree is already clean (48 C/C++ files, zero findings with Exuberant Ctags 5.9, the CI version), so the check turns on without touching any source file.
🤖 Generated with Claude Code