[CI] Restore the per-function Doxygen check - #688

Open
myungjoo wants to merge 1 commit into
nnstreamer:mainfrom
myungjoo:ci/doxygen-function-check
Open

[CI] Restore the per-function Doxygen check#688
myungjoo wants to merge 1 commit into
nnstreamer:mainfrom
myungjoo:ci/doxygen-function-check

Conversation

@myungjoo

Copy link
Copy Markdown
Member

Port of nnstreamer/nnstreamer#4914 (issue nnstreamer/nnstreamer#4908).

What was wrong

.github/workflows/static.check.scripts/doxygen-tag.sh here is byte-identical to the nnstreamer copy: local function_check_flag="f+p" at script top level, which bash rejects, and no set -e, so ctags -x --c-kinds= ran with an empty kind list and the per-function @brief check has passed vacuously since the Actions import. $report_path was never set (an "ambiguous redirect" per file on stderr) and $brief was 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.yml with the same merge-ref guard:

  • definitions are checked everywhere, prototypes only in headers;
  • the trailing /**< ... */ form is recognised (declaration line, a line opening with /**< right after it, or after an inline body);
  • block comments are tracked so a * at the start of a wrapped parameter line is not taken for a comment continuation;
  • report_path defaults to /dev/null, brief resets per file;
  • test_doxygen_tag.sh generates 18 fixtures into a temp dir; all 18 fail against the checker on main and 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

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>
@myungjoo

Copy link
Copy Markdown
MemberAuthor

[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 (gh pr view 4914 --repo nnstreamer/nnstreamer --comments, 4 review rounds), diffed this PR (upstream/main...HEAD, 3 files, +325/-8) and byte-compared doxygen-tag.sh/test_doxygen_tag.sh against nnstreamer's final approved commit 4490a668 (identical modulo CRLF), read static.check.yml's new step and the rest of the job, and independently re-verified the PR's "zero backlog" claim against this repo's own 48 C/C++ files (ctags is not available in my sandbox, so I wrote and sanity-checked a Python heuristic against the checker's actual state-machine rules, cross-checked with gh pr checks / gh run view --log). Also checked nnstreamer/api's branch protection via the GitHub API.

Does it solve the stated problem

Yes. doxygen-tag.sh here was byte-identical to nnstreamer's pre-#4914 copy, carrying the same top-level local function_check_flag="f+p" (invalid outside a function, silently emptying ctags's kind list with no set -e to catch it), the same unset $report_path, and the same missing per-file reset of $brief. This PR takes nnstreamer's already-fixed, 4-round-reviewed version verbatim — I confirmed diff -u --strip-trailing-cr between this PR's two scripts and nnstreamer PR #4914's head (4490a668) is empty. That version's design (definitions checked everywhere, prototypes only in .h/.hh/.hpp; trailing /**< recognition on same line, next line, or after an inline body; real block-comment tracking via in_comment instead of a bare-* heuristic) was independently traced and stress-tested across 4 review rounds on the origin PR, including two real counterexamples found and fixed in-tree (gsttensor_converter.c's wrapped-pointer prototype, Looper::post getting credited for a sibling's trailing /**<). Nothing in this port reopens either of those.

Backlog = 0 claim — verified independently

I could not run ctags locally (not installed, no sudo in my sandbox), so instead of trusting the PR body I wrote a Python approximation of the checker's own rules (K&R return-type-on-own-line handling, @brief/@copydoc block detection, trailing /**< on the same/next line) and sanity-checked it against a synthetic documented/undocumented pair before trusting its output on the real tree.

  • Whole-tree scan (48 files): 1141 candidate function definition/declaration sites, 0 flagged as undocumented.
  • Header-prototype-only scan (f+p scope, 9 header files): 216 candidate prototypes, 0 flagged.
  • Every one of the 438 GTest TEST (suite, case) macros across tests/capi/*.cc (these get tagged as ordinary functions by ctags' generic C parser) is immediately preceded by a comment block containing @brief, verified with a separate direct script, not just the heuristic above.
  • Every named-tag struct/class in the tree (_ml_pipeline, _ml_pipeline_element, _ml_pipeline_common_elem, _type_int, _type_string, _type_element, _conf_detail, _camera_conf) has its own leading @brief.
  • This matches the PR's own CI: the self-test step ran with the real exuberant-ctags the "Preparing step 2" installs, and all 18 PASS: lines plus test_doxygen_tag.sh: all checks passed. appear in the live log of run 33715005844.

I did not find a counterexample. The claim holds as far as I can verify it.

Self-test / CI wiring

test_doxygen_tag.sh (18 fixtures) is a solid regression suite: it pins the negative control (undocumented definition must fail — the exact property that was silently broken for years), both scope halves (f vs f+p), all three /**< placements plus the two adjacency traps found in the origin review (trailing_belongs_to_next.hh, pointer_line.h/wrapped_pointer.h), the per-file state reset in both directions, and asserts zero stderr output (which is how the original local bug and the unset report_path actually manifested). It generates fixtures into mktemp -d rather than committing them, so an intentionally-undocumented fixture never lands in a future PR's $changed_file_list.

Positive note on the port, not an issue: in nnstreamer's static.check.yml this same self-test step's comment says "needs the same merge-ref guard as the self-test steps below" — there it's one of five sibling self-test steps sharing the pattern. nnstreamer/api's static.check.yml has no other self-test step, and the author rewrote the comment here to be self-contained (explaining why the merge-ref-vs-checkout-ref mismatch can happen, rather than pointing at nonexistent siblings). I checked — there is genuinely no other diff-filter=D/self-test step anywhere in this repo's .github/workflows/, so the rewritten comment is the correct call, not an oversight.

should-fix (not blocking this PR — outside its diff, but answers a question this review was asked to check)

nnstreamer/api's branch protection on main does not include "Static checks" as a required status check (gh api repos/nnstreamer/api/branches/main/protectionrequired_status_checks.contexts: ["DCO"] only). This predates this PR — the "Static checks" job already existed with a dozen other checkers before this change — and it isn't something this PR's diff can fix (the repo has no .github/settings.yml or other repo-as-code branch-protection file; it's a GitHub UI/API-only setting). But it directly bears on the review question "can CI failing actually block a future regression from merging": today, a red Static checks job (including a future regression this exact self-test would catch) does not by itself block the merge button, only a red/missing DCO does. Recommend the maintainer add Static checks to the required status checks for main in a follow-up (not part of this PR).

Informational / no PR action needed

  • Spell Check with Typos is currently failing on this PR's CI, but on a pre-existing typo ("gievn" → "given") in c/src/ml-api-service-offloading.c:306, introduced in commit 751f51b6 (2023-06-30), a file this PR does not touch. Confirmed via git blame against upstream/main. Unrelated to this change.
  • The known, already-accepted-upstream heuristic limits of has_trailing_doc/in_comment (can't distinguish a real /* from one inside a string literal or a x / *p expression without a space) carry over unchanged from the already-reviewed nnstreamer version; no live instance of either pattern exists in this tree today (checked by grep), same conclusion the origin reviews reached for nnstreamer's tree.
  • has_trailing_doc accepts any /**< without requiring @brief/@copydoc inside it, which is a slightly lower bar than the leading-comment form — again unchanged from the reviewed-and-approved upstream version, and consistent with how this repo's own headers already use /**< on enum/struct members.

Size / scope

Proportionate and minimal: 3 files, all under .github/workflows/, no source file touched (the tree already being clean under the corrected scope means there's no comment-only backlog-clearing commit needed here, unlike the origin PR's 34 files). No unrelated module is touched. Single commit, DCO-signed, subject line follows [Component] Summary convention, body explains what/why and credits the upstream fix.

Architecture / API docs

None needed. CI-script-only change; this repo has no CLAUDE.md/AGENTS.md/Documentation/ referencing the doxygen-check policy that this PR's scope choices would need to update.

CI status at review time

Static checks: pass (run 33715005844), self-test 18/18 PASS, doxygen-tag check itself ran clean over the 2 changed files. DCO: pass. Spell Check with Typos: fail, pre-existing/unrelated (see above). build (armeabi-v7a): pass. Remaining build (*)/Tizen GBS jobs were still pending/queued at review time; this PR touches no source or build file, so I have no reason to expect them to be affected, but I did not wait for them to finish.

Verdict

Mergeable 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.

@myungjoo
myungjoo marked this pull request as ready for review September 3, 2026 05:06
jaeyun-jung pushed a commit that referenced this pull request Sep 3, 2026
"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>
Sign up for freeto 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.

1 participant

@myungjoo
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all
 blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks");
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '
Skip to content

[CI] Restore the per-function Doxygen check - #688

Open
myungjoo wants to merge 1 commit into
nnstreamer:mainfrom
myungjoo:ci/doxygen-function-check
Open

[CI] Restore the per-function Doxygen check#688
myungjoo wants to merge 1 commit into
nnstreamer:mainfrom
myungjoo:ci/doxygen-function-check

Conversation

@myungjoo

Copy link
Copy Markdown
Member

Port of nnstreamer/nnstreamer#4914 (issue nnstreamer/nnstreamer#4908).

What was wrong

.github/workflows/static.check.scripts/doxygen-tag.sh here is byte-identical to the nnstreamer copy: local function_check_flag="f+p" at script top level, which bash rejects, and no set -e, so ctags -x --c-kinds= ran with an empty kind list and the per-function @brief check has passed vacuously since the Actions import. $report_path was never set (an "ambiguous redirect" per file on stderr) and $brief was 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.yml with the same merge-ref guard:

  • definitions are checked everywhere, prototypes only in headers;
  • the trailing /**< ... */ form is recognised (declaration line, a line opening with /**< right after it, or after an inline body);
  • block comments are tracked so a * at the start of a wrapped parameter line is not taken for a comment continuation;
  • report_path defaults to /dev/null, brief resets per file;
  • test_doxygen_tag.sh generates 18 fixtures into a temp dir; all 18 fail against the checker on main and 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

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>
@myungjoo

Copy link
Copy Markdown
MemberAuthor

[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 (gh pr view 4914 --repo nnstreamer/nnstreamer --comments, 4 review rounds), diffed this PR (upstream/main...HEAD, 3 files, +325/-8) and byte-compared doxygen-tag.sh/test_doxygen_tag.sh against nnstreamer's final approved commit 4490a668 (identical modulo CRLF), read static.check.yml's new step and the rest of the job, and independently re-verified the PR's "zero backlog" claim against this repo's own 48 C/C++ files (ctags is not available in my sandbox, so I wrote and sanity-checked a Python heuristic against the checker's actual state-machine rules, cross-checked with gh pr checks / gh run view --log). Also checked nnstreamer/api's branch protection via the GitHub API.

Does it solve the stated problem

Yes. doxygen-tag.sh here was byte-identical to nnstreamer's pre-#4914 copy, carrying the same top-level local function_check_flag="f+p" (invalid outside a function, silently emptying ctags's kind list with no set -e to catch it), the same unset $report_path, and the same missing per-file reset of $brief. This PR takes nnstreamer's already-fixed, 4-round-reviewed version verbatim — I confirmed diff -u --strip-trailing-cr between this PR's two scripts and nnstreamer PR #4914's head (4490a668) is empty. That version's design (definitions checked everywhere, prototypes only in .h/.hh/.hpp; trailing /**< recognition on same line, next line, or after an inline body; real block-comment tracking via in_comment instead of a bare-* heuristic) was independently traced and stress-tested across 4 review rounds on the origin PR, including two real counterexamples found and fixed in-tree (gsttensor_converter.c's wrapped-pointer prototype, Looper::post getting credited for a sibling's trailing /**<). Nothing in this port reopens either of those.

Backlog = 0 claim — verified independently

I could not run ctags locally (not installed, no sudo in my sandbox), so instead of trusting the PR body I wrote a Python approximation of the checker's own rules (K&R return-type-on-own-line handling, @brief/@copydoc block detection, trailing /**< on the same/next line) and sanity-checked it against a synthetic documented/undocumented pair before trusting its output on the real tree.

  • Whole-tree scan (48 files): 1141 candidate function definition/declaration sites, 0 flagged as undocumented.
  • Header-prototype-only scan (f+p scope, 9 header files): 216 candidate prototypes, 0 flagged.
  • Every one of the 438 GTest TEST (suite, case) macros across tests/capi/*.cc (these get tagged as ordinary functions by ctags' generic C parser) is immediately preceded by a comment block containing @brief, verified with a separate direct script, not just the heuristic above.
  • Every named-tag struct/class in the tree (_ml_pipeline, _ml_pipeline_element, _ml_pipeline_common_elem, _type_int, _type_string, _type_element, _conf_detail, _camera_conf) has its own leading @brief.
  • This matches the PR's own CI: the self-test step ran with the real exuberant-ctags the "Preparing step 2" installs, and all 18 PASS: lines plus test_doxygen_tag.sh: all checks passed. appear in the live log of run 33715005844.

I did not find a counterexample. The claim holds as far as I can verify it.

Self-test / CI wiring

test_doxygen_tag.sh (18 fixtures) is a solid regression suite: it pins the negative control (undocumented definition must fail — the exact property that was silently broken for years), both scope halves (f vs f+p), all three /**< placements plus the two adjacency traps found in the origin review (trailing_belongs_to_next.hh, pointer_line.h/wrapped_pointer.h), the per-file state reset in both directions, and asserts zero stderr output (which is how the original local bug and the unset report_path actually manifested). It generates fixtures into mktemp -d rather than committing them, so an intentionally-undocumented fixture never lands in a future PR's $changed_file_list.

Positive note on the port, not an issue: in nnstreamer's static.check.yml this same self-test step's comment says "needs the same merge-ref guard as the self-test steps below" — there it's one of five sibling self-test steps sharing the pattern. nnstreamer/api's static.check.yml has no other self-test step, and the author rewrote the comment here to be self-contained (explaining why the merge-ref-vs-checkout-ref mismatch can happen, rather than pointing at nonexistent siblings). I checked — there is genuinely no other diff-filter=D/self-test step anywhere in this repo's .github/workflows/, so the rewritten comment is the correct call, not an oversight.

should-fix (not blocking this PR — outside its diff, but answers a question this review was asked to check)

nnstreamer/api's branch protection on main does not include "Static checks" as a required status check (gh api repos/nnstreamer/api/branches/main/protectionrequired_status_checks.contexts: ["DCO"] only). This predates this PR — the "Static checks" job already existed with a dozen other checkers before this change — and it isn't something this PR's diff can fix (the repo has no .github/settings.yml or other repo-as-code branch-protection file; it's a GitHub UI/API-only setting). But it directly bears on the review question "can CI failing actually block a future regression from merging": today, a red Static checks job (including a future regression this exact self-test would catch) does not by itself block the merge button, only a red/missing DCO does. Recommend the maintainer add Static checks to the required status checks for main in a follow-up (not part of this PR).

Informational / no PR action needed

  • Spell Check with Typos is currently failing on this PR's CI, but on a pre-existing typo ("gievn" → "given") in c/src/ml-api-service-offloading.c:306, introduced in commit 751f51b6 (2023-06-30), a file this PR does not touch. Confirmed via git blame against upstream/main. Unrelated to this change.
  • The known, already-accepted-upstream heuristic limits of has_trailing_doc/in_comment (can't distinguish a real /* from one inside a string literal or a x / *p expression without a space) carry over unchanged from the already-reviewed nnstreamer version; no live instance of either pattern exists in this tree today (checked by grep), same conclusion the origin reviews reached for nnstreamer's tree.
  • has_trailing_doc accepts any /**< without requiring @brief/@copydoc inside it, which is a slightly lower bar than the leading-comment form — again unchanged from the reviewed-and-approved upstream version, and consistent with how this repo's own headers already use /**< on enum/struct members.

Size / scope

Proportionate and minimal: 3 files, all under .github/workflows/, no source file touched (the tree already being clean under the corrected scope means there's no comment-only backlog-clearing commit needed here, unlike the origin PR's 34 files). No unrelated module is touched. Single commit, DCO-signed, subject line follows [Component] Summary convention, body explains what/why and credits the upstream fix.

Architecture / API docs

None needed. CI-script-only change; this repo has no CLAUDE.md/AGENTS.md/Documentation/ referencing the doxygen-check policy that this PR's scope choices would need to update.

CI status at review time

Static checks: pass (run 33715005844), self-test 18/18 PASS, doxygen-tag check itself ran clean over the 2 changed files. DCO: pass. Spell Check with Typos: fail, pre-existing/unrelated (see above). build (armeabi-v7a): pass. Remaining build (*)/Tizen GBS jobs were still pending/queued at review time; this PR touches no source or build file, so I have no reason to expect them to be affected, but I did not wait for them to finish.

Verdict

Mergeable 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.

@myungjoo
myungjoo marked this pull request as ready for review September 3, 2026 05:06
jaeyun-jung pushed a commit that referenced this pull request Sep 3, 2026
"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>
Sign up for freeto 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.

1 participant

@myungjoo
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix"); } } catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

[CI] Restore the per-function Doxygen check - #688

Open
myungjoo wants to merge 1 commit into
nnstreamer:mainfrom
myungjoo:ci/doxygen-function-check
Open

[CI] Restore the per-function Doxygen check#688
myungjoo wants to merge 1 commit into
nnstreamer:mainfrom
myungjoo:ci/doxygen-function-check

Conversation

@myungjoo

Copy link
Copy Markdown
Member

Port of nnstreamer/nnstreamer#4914 (issue nnstreamer/nnstreamer#4908).

What was wrong

.github/workflows/static.check.scripts/doxygen-tag.sh here is byte-identical to the nnstreamer copy: local function_check_flag="f+p" at script top level, which bash rejects, and no set -e, so ctags -x --c-kinds= ran with an empty kind list and the per-function @brief check has passed vacuously since the Actions import. $report_path was never set (an "ambiguous redirect" per file on stderr) and $brief was 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.yml with the same merge-ref guard:

  • definitions are checked everywhere, prototypes only in headers;
  • the trailing /**< ... */ form is recognised (declaration line, a line opening with /**< right after it, or after an inline body);
  • block comments are tracked so a * at the start of a wrapped parameter line is not taken for a comment continuation;
  • report_path defaults to /dev/null, brief resets per file;
  • test_doxygen_tag.sh generates 18 fixtures into a temp dir; all 18 fail against the checker on main and 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

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>
@myungjoo

Copy link
Copy Markdown
MemberAuthor

[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 (gh pr view 4914 --repo nnstreamer/nnstreamer --comments, 4 review rounds), diffed this PR (upstream/main...HEAD, 3 files, +325/-8) and byte-compared doxygen-tag.sh/test_doxygen_tag.sh against nnstreamer's final approved commit 4490a668 (identical modulo CRLF), read static.check.yml's new step and the rest of the job, and independently re-verified the PR's "zero backlog" claim against this repo's own 48 C/C++ files (ctags is not available in my sandbox, so I wrote and sanity-checked a Python heuristic against the checker's actual state-machine rules, cross-checked with gh pr checks / gh run view --log). Also checked nnstreamer/api's branch protection via the GitHub API.

Does it solve the stated problem

Yes. doxygen-tag.sh here was byte-identical to nnstreamer's pre-#4914 copy, carrying the same top-level local function_check_flag="f+p" (invalid outside a function, silently emptying ctags's kind list with no set -e to catch it), the same unset $report_path, and the same missing per-file reset of $brief. This PR takes nnstreamer's already-fixed, 4-round-reviewed version verbatim — I confirmed diff -u --strip-trailing-cr between this PR's two scripts and nnstreamer PR #4914's head (4490a668) is empty. That version's design (definitions checked everywhere, prototypes only in .h/.hh/.hpp; trailing /**< recognition on same line, next line, or after an inline body; real block-comment tracking via in_comment instead of a bare-* heuristic) was independently traced and stress-tested across 4 review rounds on the origin PR, including two real counterexamples found and fixed in-tree (gsttensor_converter.c's wrapped-pointer prototype, Looper::post getting credited for a sibling's trailing /**<). Nothing in this port reopens either of those.

Backlog = 0 claim — verified independently

I could not run ctags locally (not installed, no sudo in my sandbox), so instead of trusting the PR body I wrote a Python approximation of the checker's own rules (K&R return-type-on-own-line handling, @brief/@copydoc block detection, trailing /**< on the same/next line) and sanity-checked it against a synthetic documented/undocumented pair before trusting its output on the real tree.

  • Whole-tree scan (48 files): 1141 candidate function definition/declaration sites, 0 flagged as undocumented.
  • Header-prototype-only scan (f+p scope, 9 header files): 216 candidate prototypes, 0 flagged.
  • Every one of the 438 GTest TEST (suite, case) macros across tests/capi/*.cc (these get tagged as ordinary functions by ctags' generic C parser) is immediately preceded by a comment block containing @brief, verified with a separate direct script, not just the heuristic above.
  • Every named-tag struct/class in the tree (_ml_pipeline, _ml_pipeline_element, _ml_pipeline_common_elem, _type_int, _type_string, _type_element, _conf_detail, _camera_conf) has its own leading @brief.
  • This matches the PR's own CI: the self-test step ran with the real exuberant-ctags the "Preparing step 2" installs, and all 18 PASS: lines plus test_doxygen_tag.sh: all checks passed. appear in the live log of run 33715005844.

I did not find a counterexample. The claim holds as far as I can verify it.

Self-test / CI wiring

test_doxygen_tag.sh (18 fixtures) is a solid regression suite: it pins the negative control (undocumented definition must fail — the exact property that was silently broken for years), both scope halves (f vs f+p), all three /**< placements plus the two adjacency traps found in the origin review (trailing_belongs_to_next.hh, pointer_line.h/wrapped_pointer.h), the per-file state reset in both directions, and asserts zero stderr output (which is how the original local bug and the unset report_path actually manifested). It generates fixtures into mktemp -d rather than committing them, so an intentionally-undocumented fixture never lands in a future PR's $changed_file_list.

Positive note on the port, not an issue: in nnstreamer's static.check.yml this same self-test step's comment says "needs the same merge-ref guard as the self-test steps below" — there it's one of five sibling self-test steps sharing the pattern. nnstreamer/api's static.check.yml has no other self-test step, and the author rewrote the comment here to be self-contained (explaining why the merge-ref-vs-checkout-ref mismatch can happen, rather than pointing at nonexistent siblings). I checked — there is genuinely no other diff-filter=D/self-test step anywhere in this repo's .github/workflows/, so the rewritten comment is the correct call, not an oversight.

should-fix (not blocking this PR — outside its diff, but answers a question this review was asked to check)

nnstreamer/api's branch protection on main does not include "Static checks" as a required status check (gh api repos/nnstreamer/api/branches/main/protectionrequired_status_checks.contexts: ["DCO"] only). This predates this PR — the "Static checks" job already existed with a dozen other checkers before this change — and it isn't something this PR's diff can fix (the repo has no .github/settings.yml or other repo-as-code branch-protection file; it's a GitHub UI/API-only setting). But it directly bears on the review question "can CI failing actually block a future regression from merging": today, a red Static checks job (including a future regression this exact self-test would catch) does not by itself block the merge button, only a red/missing DCO does. Recommend the maintainer add Static checks to the required status checks for main in a follow-up (not part of this PR).

Informational / no PR action needed

  • Spell Check with Typos is currently failing on this PR's CI, but on a pre-existing typo ("gievn" → "given") in c/src/ml-api-service-offloading.c:306, introduced in commit 751f51b6 (2023-06-30), a file this PR does not touch. Confirmed via git blame against upstream/main. Unrelated to this change.
  • The known, already-accepted-upstream heuristic limits of has_trailing_doc/in_comment (can't distinguish a real /* from one inside a string literal or a x / *p expression without a space) carry over unchanged from the already-reviewed nnstreamer version; no live instance of either pattern exists in this tree today (checked by grep), same conclusion the origin reviews reached for nnstreamer's tree.
  • has_trailing_doc accepts any /**< without requiring @brief/@copydoc inside it, which is a slightly lower bar than the leading-comment form — again unchanged from the reviewed-and-approved upstream version, and consistent with how this repo's own headers already use /**< on enum/struct members.

Size / scope

Proportionate and minimal: 3 files, all under .github/workflows/, no source file touched (the tree already being clean under the corrected scope means there's no comment-only backlog-clearing commit needed here, unlike the origin PR's 34 files). No unrelated module is touched. Single commit, DCO-signed, subject line follows [Component] Summary convention, body explains what/why and credits the upstream fix.

Architecture / API docs

None needed. CI-script-only change; this repo has no CLAUDE.md/AGENTS.md/Documentation/ referencing the doxygen-check policy that this PR's scope choices would need to update.

CI status at review time

Static checks: pass (run 33715005844), self-test 18/18 PASS, doxygen-tag check itself ran clean over the 2 changed files. DCO: pass. Spell Check with Typos: fail, pre-existing/unrelated (see above). build (armeabi-v7a): pass. Remaining build (*)/Tizen GBS jobs were still pending/queued at review time; this PR touches no source or build file, so I have no reason to expect them to be affected, but I did not wait for them to finish.

Verdict

Mergeable 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.

@myungjoo
myungjoo marked this pull request as ready for review September 3, 2026 05:06
jaeyun-jung pushed a commit that referenced this pull request Sep 3, 2026
"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>
Sign up for freeto 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.

1 participant

@myungjoo
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length > 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms"); } } catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

[CI] Restore the per-function Doxygen check - #688

Open
myungjoo wants to merge 1 commit into
nnstreamer:mainfrom
myungjoo:ci/doxygen-function-check
Open

[CI] Restore the per-function Doxygen check#688
myungjoo wants to merge 1 commit into
nnstreamer:mainfrom
myungjoo:ci/doxygen-function-check

Conversation

@myungjoo

Copy link
Copy Markdown
Member

Port of nnstreamer/nnstreamer#4914 (issue nnstreamer/nnstreamer#4908).

What was wrong

.github/workflows/static.check.scripts/doxygen-tag.sh here is byte-identical to the nnstreamer copy: local function_check_flag="f+p" at script top level, which bash rejects, and no set -e, so ctags -x --c-kinds= ran with an empty kind list and the per-function @brief check has passed vacuously since the Actions import. $report_path was never set (an "ambiguous redirect" per file on stderr) and $brief was 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.yml with the same merge-ref guard:

  • definitions are checked everywhere, prototypes only in headers;
  • the trailing /**< ... */ form is recognised (declaration line, a line opening with /**< right after it, or after an inline body);
  • block comments are tracked so a * at the start of a wrapped parameter line is not taken for a comment continuation;
  • report_path defaults to /dev/null, brief resets per file;
  • test_doxygen_tag.sh generates 18 fixtures into a temp dir; all 18 fail against the checker on main and 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

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>
@myungjoo

Copy link
Copy Markdown
MemberAuthor

[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 (gh pr view 4914 --repo nnstreamer/nnstreamer --comments, 4 review rounds), diffed this PR (upstream/main...HEAD, 3 files, +325/-8) and byte-compared doxygen-tag.sh/test_doxygen_tag.sh against nnstreamer's final approved commit 4490a668 (identical modulo CRLF), read static.check.yml's new step and the rest of the job, and independently re-verified the PR's "zero backlog" claim against this repo's own 48 C/C++ files (ctags is not available in my sandbox, so I wrote and sanity-checked a Python heuristic against the checker's actual state-machine rules, cross-checked with gh pr checks / gh run view --log). Also checked nnstreamer/api's branch protection via the GitHub API.

Does it solve the stated problem

Yes. doxygen-tag.sh here was byte-identical to nnstreamer's pre-#4914 copy, carrying the same top-level local function_check_flag="f+p" (invalid outside a function, silently emptying ctags's kind list with no set -e to catch it), the same unset $report_path, and the same missing per-file reset of $brief. This PR takes nnstreamer's already-fixed, 4-round-reviewed version verbatim — I confirmed diff -u --strip-trailing-cr between this PR's two scripts and nnstreamer PR #4914's head (4490a668) is empty. That version's design (definitions checked everywhere, prototypes only in .h/.hh/.hpp; trailing /**< recognition on same line, next line, or after an inline body; real block-comment tracking via in_comment instead of a bare-* heuristic) was independently traced and stress-tested across 4 review rounds on the origin PR, including two real counterexamples found and fixed in-tree (gsttensor_converter.c's wrapped-pointer prototype, Looper::post getting credited for a sibling's trailing /**<). Nothing in this port reopens either of those.

Backlog = 0 claim — verified independently

I could not run ctags locally (not installed, no sudo in my sandbox), so instead of trusting the PR body I wrote a Python approximation of the checker's own rules (K&R return-type-on-own-line handling, @brief/@copydoc block detection, trailing /**< on the same/next line) and sanity-checked it against a synthetic documented/undocumented pair before trusting its output on the real tree.

  • Whole-tree scan (48 files): 1141 candidate function definition/declaration sites, 0 flagged as undocumented.
  • Header-prototype-only scan (f+p scope, 9 header files): 216 candidate prototypes, 0 flagged.
  • Every one of the 438 GTest TEST (suite, case) macros across tests/capi/*.cc (these get tagged as ordinary functions by ctags' generic C parser) is immediately preceded by a comment block containing @brief, verified with a separate direct script, not just the heuristic above.
  • Every named-tag struct/class in the tree (_ml_pipeline, _ml_pipeline_element, _ml_pipeline_common_elem, _type_int, _type_string, _type_element, _conf_detail, _camera_conf) has its own leading @brief.
  • This matches the PR's own CI: the self-test step ran with the real exuberant-ctags the "Preparing step 2" installs, and all 18 PASS: lines plus test_doxygen_tag.sh: all checks passed. appear in the live log of run 33715005844.

I did not find a counterexample. The claim holds as far as I can verify it.

Self-test / CI wiring

test_doxygen_tag.sh (18 fixtures) is a solid regression suite: it pins the negative control (undocumented definition must fail — the exact property that was silently broken for years), both scope halves (f vs f+p), all three /**< placements plus the two adjacency traps found in the origin review (trailing_belongs_to_next.hh, pointer_line.h/wrapped_pointer.h), the per-file state reset in both directions, and asserts zero stderr output (which is how the original local bug and the unset report_path actually manifested). It generates fixtures into mktemp -d rather than committing them, so an intentionally-undocumented fixture never lands in a future PR's $changed_file_list.

Positive note on the port, not an issue: in nnstreamer's static.check.yml this same self-test step's comment says "needs the same merge-ref guard as the self-test steps below" — there it's one of five sibling self-test steps sharing the pattern. nnstreamer/api's static.check.yml has no other self-test step, and the author rewrote the comment here to be self-contained (explaining why the merge-ref-vs-checkout-ref mismatch can happen, rather than pointing at nonexistent siblings). I checked — there is genuinely no other diff-filter=D/self-test step anywhere in this repo's .github/workflows/, so the rewritten comment is the correct call, not an oversight.

should-fix (not blocking this PR — outside its diff, but answers a question this review was asked to check)

nnstreamer/api's branch protection on main does not include "Static checks" as a required status check (gh api repos/nnstreamer/api/branches/main/protectionrequired_status_checks.contexts: ["DCO"] only). This predates this PR — the "Static checks" job already existed with a dozen other checkers before this change — and it isn't something this PR's diff can fix (the repo has no .github/settings.yml or other repo-as-code branch-protection file; it's a GitHub UI/API-only setting). But it directly bears on the review question "can CI failing actually block a future regression from merging": today, a red Static checks job (including a future regression this exact self-test would catch) does not by itself block the merge button, only a red/missing DCO does. Recommend the maintainer add Static checks to the required status checks for main in a follow-up (not part of this PR).

Informational / no PR action needed

  • Spell Check with Typos is currently failing on this PR's CI, but on a pre-existing typo ("gievn" → "given") in c/src/ml-api-service-offloading.c:306, introduced in commit 751f51b6 (2023-06-30), a file this PR does not touch. Confirmed via git blame against upstream/main. Unrelated to this change.
  • The known, already-accepted-upstream heuristic limits of has_trailing_doc/in_comment (can't distinguish a real /* from one inside a string literal or a x / *p expression without a space) carry over unchanged from the already-reviewed nnstreamer version; no live instance of either pattern exists in this tree today (checked by grep), same conclusion the origin reviews reached for nnstreamer's tree.
  • has_trailing_doc accepts any /**< without requiring @brief/@copydoc inside it, which is a slightly lower bar than the leading-comment form — again unchanged from the reviewed-and-approved upstream version, and consistent with how this repo's own headers already use /**< on enum/struct members.

Size / scope

Proportionate and minimal: 3 files, all under .github/workflows/, no source file touched (the tree already being clean under the corrected scope means there's no comment-only backlog-clearing commit needed here, unlike the origin PR's 34 files). No unrelated module is touched. Single commit, DCO-signed, subject line follows [Component] Summary convention, body explains what/why and credits the upstream fix.

Architecture / API docs

None needed. CI-script-only change; this repo has no CLAUDE.md/AGENTS.md/Documentation/ referencing the doxygen-check policy that this PR's scope choices would need to update.

CI status at review time

Static checks: pass (run 33715005844), self-test 18/18 PASS, doxygen-tag check itself ran clean over the 2 changed files. DCO: pass. Spell Check with Typos: fail, pre-existing/unrelated (see above). build (armeabi-v7a): pass. Remaining build (*)/Tizen GBS jobs were still pending/queued at review time; this PR touches no source or build file, so I have no reason to expect them to be affected, but I did not wait for them to finish.

Verdict

Mergeable 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.

@myungjoo
myungjoo marked this pull request as ready for review September 3, 2026 05:06
jaeyun-jung pushed a commit that referenced this pull request Sep 3, 2026
"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>
Sign up for freeto 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.

1 participant

@myungjoo
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links"); } } catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); } })(); (function(){ try { var __m = "youtube.com"; var __re = new RegExp('^' + "youtube\\.com" + '
Skip to content

[CI] Restore the per-function Doxygen check - #688

Open
myungjoo wants to merge 1 commit into
nnstreamer:mainfrom
myungjoo:ci/doxygen-function-check
Open

[CI] Restore the per-function Doxygen check#688
myungjoo wants to merge 1 commit into
nnstreamer:mainfrom
myungjoo:ci/doxygen-function-check

Conversation

@myungjoo

Copy link
Copy Markdown
Member

Port of nnstreamer/nnstreamer#4914 (issue nnstreamer/nnstreamer#4908).

What was wrong

.github/workflows/static.check.scripts/doxygen-tag.sh here is byte-identical to the nnstreamer copy: local function_check_flag="f+p" at script top level, which bash rejects, and no set -e, so ctags -x --c-kinds= ran with an empty kind list and the per-function @brief check has passed vacuously since the Actions import. $report_path was never set (an "ambiguous redirect" per file on stderr) and $brief was 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.yml with the same merge-ref guard:

  • definitions are checked everywhere, prototypes only in headers;
  • the trailing /**< ... */ form is recognised (declaration line, a line opening with /**< right after it, or after an inline body);
  • block comments are tracked so a * at the start of a wrapped parameter line is not taken for a comment continuation;
  • report_path defaults to /dev/null, brief resets per file;
  • test_doxygen_tag.sh generates 18 fixtures into a temp dir; all 18 fail against the checker on main and 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

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>
@myungjoo

Copy link
Copy Markdown
MemberAuthor

[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 (gh pr view 4914 --repo nnstreamer/nnstreamer --comments, 4 review rounds), diffed this PR (upstream/main...HEAD, 3 files, +325/-8) and byte-compared doxygen-tag.sh/test_doxygen_tag.sh against nnstreamer's final approved commit 4490a668 (identical modulo CRLF), read static.check.yml's new step and the rest of the job, and independently re-verified the PR's "zero backlog" claim against this repo's own 48 C/C++ files (ctags is not available in my sandbox, so I wrote and sanity-checked a Python heuristic against the checker's actual state-machine rules, cross-checked with gh pr checks / gh run view --log). Also checked nnstreamer/api's branch protection via the GitHub API.

Does it solve the stated problem

Yes. doxygen-tag.sh here was byte-identical to nnstreamer's pre-#4914 copy, carrying the same top-level local function_check_flag="f+p" (invalid outside a function, silently emptying ctags's kind list with no set -e to catch it), the same unset $report_path, and the same missing per-file reset of $brief. This PR takes nnstreamer's already-fixed, 4-round-reviewed version verbatim — I confirmed diff -u --strip-trailing-cr between this PR's two scripts and nnstreamer PR #4914's head (4490a668) is empty. That version's design (definitions checked everywhere, prototypes only in .h/.hh/.hpp; trailing /**< recognition on same line, next line, or after an inline body; real block-comment tracking via in_comment instead of a bare-* heuristic) was independently traced and stress-tested across 4 review rounds on the origin PR, including two real counterexamples found and fixed in-tree (gsttensor_converter.c's wrapped-pointer prototype, Looper::post getting credited for a sibling's trailing /**<). Nothing in this port reopens either of those.

Backlog = 0 claim — verified independently

I could not run ctags locally (not installed, no sudo in my sandbox), so instead of trusting the PR body I wrote a Python approximation of the checker's own rules (K&R return-type-on-own-line handling, @brief/@copydoc block detection, trailing /**< on the same/next line) and sanity-checked it against a synthetic documented/undocumented pair before trusting its output on the real tree.

  • Whole-tree scan (48 files): 1141 candidate function definition/declaration sites, 0 flagged as undocumented.
  • Header-prototype-only scan (f+p scope, 9 header files): 216 candidate prototypes, 0 flagged.
  • Every one of the 438 GTest TEST (suite, case) macros across tests/capi/*.cc (these get tagged as ordinary functions by ctags' generic C parser) is immediately preceded by a comment block containing @brief, verified with a separate direct script, not just the heuristic above.
  • Every named-tag struct/class in the tree (_ml_pipeline, _ml_pipeline_element, _ml_pipeline_common_elem, _type_int, _type_string, _type_element, _conf_detail, _camera_conf) has its own leading @brief.
  • This matches the PR's own CI: the self-test step ran with the real exuberant-ctags the "Preparing step 2" installs, and all 18 PASS: lines plus test_doxygen_tag.sh: all checks passed. appear in the live log of run 33715005844.

I did not find a counterexample. The claim holds as far as I can verify it.

Self-test / CI wiring

test_doxygen_tag.sh (18 fixtures) is a solid regression suite: it pins the negative control (undocumented definition must fail — the exact property that was silently broken for years), both scope halves (f vs f+p), all three /**< placements plus the two adjacency traps found in the origin review (trailing_belongs_to_next.hh, pointer_line.h/wrapped_pointer.h), the per-file state reset in both directions, and asserts zero stderr output (which is how the original local bug and the unset report_path actually manifested). It generates fixtures into mktemp -d rather than committing them, so an intentionally-undocumented fixture never lands in a future PR's $changed_file_list.

Positive note on the port, not an issue: in nnstreamer's static.check.yml this same self-test step's comment says "needs the same merge-ref guard as the self-test steps below" — there it's one of five sibling self-test steps sharing the pattern. nnstreamer/api's static.check.yml has no other self-test step, and the author rewrote the comment here to be self-contained (explaining why the merge-ref-vs-checkout-ref mismatch can happen, rather than pointing at nonexistent siblings). I checked — there is genuinely no other diff-filter=D/self-test step anywhere in this repo's .github/workflows/, so the rewritten comment is the correct call, not an oversight.

should-fix (not blocking this PR — outside its diff, but answers a question this review was asked to check)

nnstreamer/api's branch protection on main does not include "Static checks" as a required status check (gh api repos/nnstreamer/api/branches/main/protectionrequired_status_checks.contexts: ["DCO"] only). This predates this PR — the "Static checks" job already existed with a dozen other checkers before this change — and it isn't something this PR's diff can fix (the repo has no .github/settings.yml or other repo-as-code branch-protection file; it's a GitHub UI/API-only setting). But it directly bears on the review question "can CI failing actually block a future regression from merging": today, a red Static checks job (including a future regression this exact self-test would catch) does not by itself block the merge button, only a red/missing DCO does. Recommend the maintainer add Static checks to the required status checks for main in a follow-up (not part of this PR).

Informational / no PR action needed

  • Spell Check with Typos is currently failing on this PR's CI, but on a pre-existing typo ("gievn" → "given") in c/src/ml-api-service-offloading.c:306, introduced in commit 751f51b6 (2023-06-30), a file this PR does not touch. Confirmed via git blame against upstream/main. Unrelated to this change.
  • The known, already-accepted-upstream heuristic limits of has_trailing_doc/in_comment (can't distinguish a real /* from one inside a string literal or a x / *p expression without a space) carry over unchanged from the already-reviewed nnstreamer version; no live instance of either pattern exists in this tree today (checked by grep), same conclusion the origin reviews reached for nnstreamer's tree.
  • has_trailing_doc accepts any /**< without requiring @brief/@copydoc inside it, which is a slightly lower bar than the leading-comment form — again unchanged from the reviewed-and-approved upstream version, and consistent with how this repo's own headers already use /**< on enum/struct members.

Size / scope

Proportionate and minimal: 3 files, all under .github/workflows/, no source file touched (the tree already being clean under the corrected scope means there's no comment-only backlog-clearing commit needed here, unlike the origin PR's 34 files). No unrelated module is touched. Single commit, DCO-signed, subject line follows [Component] Summary convention, body explains what/why and credits the upstream fix.

Architecture / API docs

None needed. CI-script-only change; this repo has no CLAUDE.md/AGENTS.md/Documentation/ referencing the doxygen-check policy that this PR's scope choices would need to update.

CI status at review time

Static checks: pass (run 33715005844), self-test 18/18 PASS, doxygen-tag check itself ran clean over the 2 changed files. DCO: pass. Spell Check with Typos: fail, pre-existing/unrelated (see above). build (armeabi-v7a): pass. Remaining build (*)/Tizen GBS jobs were still pending/queued at review time; this PR touches no source or build file, so I have no reason to expect them to be affected, but I did not wait for them to finish.

Verdict

Mergeable 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.

@myungjoo
myungjoo marked this pull request as ready for review September 3, 2026 05:06
jaeyun-jung pushed a commit that referenced this pull request Sep 3, 2026
"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>
Sign up for freeto 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.

1 participant

@myungjoo
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default"); } } catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

[CI] Restore the per-function Doxygen check - #688

Open
myungjoo wants to merge 1 commit into
nnstreamer:mainfrom
myungjoo:ci/doxygen-function-check
Open

[CI] Restore the per-function Doxygen check#688
myungjoo wants to merge 1 commit into
nnstreamer:mainfrom
myungjoo:ci/doxygen-function-check

Conversation

@myungjoo

Copy link
Copy Markdown
Member

Port of nnstreamer/nnstreamer#4914 (issue nnstreamer/nnstreamer#4908).

What was wrong

.github/workflows/static.check.scripts/doxygen-tag.sh here is byte-identical to the nnstreamer copy: local function_check_flag="f+p" at script top level, which bash rejects, and no set -e, so ctags -x --c-kinds= ran with an empty kind list and the per-function @brief check has passed vacuously since the Actions import. $report_path was never set (an "ambiguous redirect" per file on stderr) and $brief was 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.yml with the same merge-ref guard:

  • definitions are checked everywhere, prototypes only in headers;
  • the trailing /**< ... */ form is recognised (declaration line, a line opening with /**< right after it, or after an inline body);
  • block comments are tracked so a * at the start of a wrapped parameter line is not taken for a comment continuation;
  • report_path defaults to /dev/null, brief resets per file;
  • test_doxygen_tag.sh generates 18 fixtures into a temp dir; all 18 fail against the checker on main and 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

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>
@myungjoo

Copy link
Copy Markdown
MemberAuthor

[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 (gh pr view 4914 --repo nnstreamer/nnstreamer --comments, 4 review rounds), diffed this PR (upstream/main...HEAD, 3 files, +325/-8) and byte-compared doxygen-tag.sh/test_doxygen_tag.sh against nnstreamer's final approved commit 4490a668 (identical modulo CRLF), read static.check.yml's new step and the rest of the job, and independently re-verified the PR's "zero backlog" claim against this repo's own 48 C/C++ files (ctags is not available in my sandbox, so I wrote and sanity-checked a Python heuristic against the checker's actual state-machine rules, cross-checked with gh pr checks / gh run view --log). Also checked nnstreamer/api's branch protection via the GitHub API.

Does it solve the stated problem

Yes. doxygen-tag.sh here was byte-identical to nnstreamer's pre-#4914 copy, carrying the same top-level local function_check_flag="f+p" (invalid outside a function, silently emptying ctags's kind list with no set -e to catch it), the same unset $report_path, and the same missing per-file reset of $brief. This PR takes nnstreamer's already-fixed, 4-round-reviewed version verbatim — I confirmed diff -u --strip-trailing-cr between this PR's two scripts and nnstreamer PR #4914's head (4490a668) is empty. That version's design (definitions checked everywhere, prototypes only in .h/.hh/.hpp; trailing /**< recognition on same line, next line, or after an inline body; real block-comment tracking via in_comment instead of a bare-* heuristic) was independently traced and stress-tested across 4 review rounds on the origin PR, including two real counterexamples found and fixed in-tree (gsttensor_converter.c's wrapped-pointer prototype, Looper::post getting credited for a sibling's trailing /**<). Nothing in this port reopens either of those.

Backlog = 0 claim — verified independently

I could not run ctags locally (not installed, no sudo in my sandbox), so instead of trusting the PR body I wrote a Python approximation of the checker's own rules (K&R return-type-on-own-line handling, @brief/@copydoc block detection, trailing /**< on the same/next line) and sanity-checked it against a synthetic documented/undocumented pair before trusting its output on the real tree.

  • Whole-tree scan (48 files): 1141 candidate function definition/declaration sites, 0 flagged as undocumented.
  • Header-prototype-only scan (f+p scope, 9 header files): 216 candidate prototypes, 0 flagged.
  • Every one of the 438 GTest TEST (suite, case) macros across tests/capi/*.cc (these get tagged as ordinary functions by ctags' generic C parser) is immediately preceded by a comment block containing @brief, verified with a separate direct script, not just the heuristic above.
  • Every named-tag struct/class in the tree (_ml_pipeline, _ml_pipeline_element, _ml_pipeline_common_elem, _type_int, _type_string, _type_element, _conf_detail, _camera_conf) has its own leading @brief.
  • This matches the PR's own CI: the self-test step ran with the real exuberant-ctags the "Preparing step 2" installs, and all 18 PASS: lines plus test_doxygen_tag.sh: all checks passed. appear in the live log of run 33715005844.

I did not find a counterexample. The claim holds as far as I can verify it.

Self-test / CI wiring

test_doxygen_tag.sh (18 fixtures) is a solid regression suite: it pins the negative control (undocumented definition must fail — the exact property that was silently broken for years), both scope halves (f vs f+p), all three /**< placements plus the two adjacency traps found in the origin review (trailing_belongs_to_next.hh, pointer_line.h/wrapped_pointer.h), the per-file state reset in both directions, and asserts zero stderr output (which is how the original local bug and the unset report_path actually manifested). It generates fixtures into mktemp -d rather than committing them, so an intentionally-undocumented fixture never lands in a future PR's $changed_file_list.

Positive note on the port, not an issue: in nnstreamer's static.check.yml this same self-test step's comment says "needs the same merge-ref guard as the self-test steps below" — there it's one of five sibling self-test steps sharing the pattern. nnstreamer/api's static.check.yml has no other self-test step, and the author rewrote the comment here to be self-contained (explaining why the merge-ref-vs-checkout-ref mismatch can happen, rather than pointing at nonexistent siblings). I checked — there is genuinely no other diff-filter=D/self-test step anywhere in this repo's .github/workflows/, so the rewritten comment is the correct call, not an oversight.

should-fix (not blocking this PR — outside its diff, but answers a question this review was asked to check)

nnstreamer/api's branch protection on main does not include "Static checks" as a required status check (gh api repos/nnstreamer/api/branches/main/protectionrequired_status_checks.contexts: ["DCO"] only). This predates this PR — the "Static checks" job already existed with a dozen other checkers before this change — and it isn't something this PR's diff can fix (the repo has no .github/settings.yml or other repo-as-code branch-protection file; it's a GitHub UI/API-only setting). But it directly bears on the review question "can CI failing actually block a future regression from merging": today, a red Static checks job (including a future regression this exact self-test would catch) does not by itself block the merge button, only a red/missing DCO does. Recommend the maintainer add Static checks to the required status checks for main in a follow-up (not part of this PR).

Informational / no PR action needed

  • Spell Check with Typos is currently failing on this PR's CI, but on a pre-existing typo ("gievn" → "given") in c/src/ml-api-service-offloading.c:306, introduced in commit 751f51b6 (2023-06-30), a file this PR does not touch. Confirmed via git blame against upstream/main. Unrelated to this change.
  • The known, already-accepted-upstream heuristic limits of has_trailing_doc/in_comment (can't distinguish a real /* from one inside a string literal or a x / *p expression without a space) carry over unchanged from the already-reviewed nnstreamer version; no live instance of either pattern exists in this tree today (checked by grep), same conclusion the origin reviews reached for nnstreamer's tree.
  • has_trailing_doc accepts any /**< without requiring @brief/@copydoc inside it, which is a slightly lower bar than the leading-comment form — again unchanged from the reviewed-and-approved upstream version, and consistent with how this repo's own headers already use /**< on enum/struct members.

Size / scope

Proportionate and minimal: 3 files, all under .github/workflows/, no source file touched (the tree already being clean under the corrected scope means there's no comment-only backlog-clearing commit needed here, unlike the origin PR's 34 files). No unrelated module is touched. Single commit, DCO-signed, subject line follows [Component] Summary convention, body explains what/why and credits the upstream fix.

Architecture / API docs

None needed. CI-script-only change; this repo has no CLAUDE.md/AGENTS.md/Documentation/ referencing the doxygen-check policy that this PR's scope choices would need to update.

CI status at review time

Static checks: pass (run 33715005844), self-test 18/18 PASS, doxygen-tag check itself ran clean over the 2 changed files. DCO: pass. Spell Check with Typos: fail, pre-existing/unrelated (see above). build (armeabi-v7a): pass. Remaining build (*)/Tizen GBS jobs were still pending/queued at review time; this PR touches no source or build file, so I have no reason to expect them to be affected, but I did not wait for them to finish.

Verdict

Mergeable 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.

@myungjoo
myungjoo marked this pull request as ready for review September 3, 2026 05:06
jaeyun-jung pushed a commit that referenced this pull request Sep 3, 2026
"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>
Sign up for freeto 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.

1 participant

@myungjoo
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers"); } } catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

[CI] Restore the per-function Doxygen check - #688

Open
myungjoo wants to merge 1 commit into
nnstreamer:mainfrom
myungjoo:ci/doxygen-function-check
Open

[CI] Restore the per-function Doxygen check#688
myungjoo wants to merge 1 commit into
nnstreamer:mainfrom
myungjoo:ci/doxygen-function-check

Conversation

@myungjoo

Copy link
Copy Markdown
Member

Port of nnstreamer/nnstreamer#4914 (issue nnstreamer/nnstreamer#4908).

What was wrong

.github/workflows/static.check.scripts/doxygen-tag.sh here is byte-identical to the nnstreamer copy: local function_check_flag="f+p" at script top level, which bash rejects, and no set -e, so ctags -x --c-kinds= ran with an empty kind list and the per-function @brief check has passed vacuously since the Actions import. $report_path was never set (an "ambiguous redirect" per file on stderr) and $brief was 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.yml with the same merge-ref guard:

  • definitions are checked everywhere, prototypes only in headers;
  • the trailing /**< ... */ form is recognised (declaration line, a line opening with /**< right after it, or after an inline body);
  • block comments are tracked so a * at the start of a wrapped parameter line is not taken for a comment continuation;
  • report_path defaults to /dev/null, brief resets per file;
  • test_doxygen_tag.sh generates 18 fixtures into a temp dir; all 18 fail against the checker on main and 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

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>
@myungjoo

Copy link
Copy Markdown
MemberAuthor

[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 (gh pr view 4914 --repo nnstreamer/nnstreamer --comments, 4 review rounds), diffed this PR (upstream/main...HEAD, 3 files, +325/-8) and byte-compared doxygen-tag.sh/test_doxygen_tag.sh against nnstreamer's final approved commit 4490a668 (identical modulo CRLF), read static.check.yml's new step and the rest of the job, and independently re-verified the PR's "zero backlog" claim against this repo's own 48 C/C++ files (ctags is not available in my sandbox, so I wrote and sanity-checked a Python heuristic against the checker's actual state-machine rules, cross-checked with gh pr checks / gh run view --log). Also checked nnstreamer/api's branch protection via the GitHub API.

Does it solve the stated problem

Yes. doxygen-tag.sh here was byte-identical to nnstreamer's pre-#4914 copy, carrying the same top-level local function_check_flag="f+p" (invalid outside a function, silently emptying ctags's kind list with no set -e to catch it), the same unset $report_path, and the same missing per-file reset of $brief. This PR takes nnstreamer's already-fixed, 4-round-reviewed version verbatim — I confirmed diff -u --strip-trailing-cr between this PR's two scripts and nnstreamer PR #4914's head (4490a668) is empty. That version's design (definitions checked everywhere, prototypes only in .h/.hh/.hpp; trailing /**< recognition on same line, next line, or after an inline body; real block-comment tracking via in_comment instead of a bare-* heuristic) was independently traced and stress-tested across 4 review rounds on the origin PR, including two real counterexamples found and fixed in-tree (gsttensor_converter.c's wrapped-pointer prototype, Looper::post getting credited for a sibling's trailing /**<). Nothing in this port reopens either of those.

Backlog = 0 claim — verified independently

I could not run ctags locally (not installed, no sudo in my sandbox), so instead of trusting the PR body I wrote a Python approximation of the checker's own rules (K&R return-type-on-own-line handling, @brief/@copydoc block detection, trailing /**< on the same/next line) and sanity-checked it against a synthetic documented/undocumented pair before trusting its output on the real tree.

  • Whole-tree scan (48 files): 1141 candidate function definition/declaration sites, 0 flagged as undocumented.
  • Header-prototype-only scan (f+p scope, 9 header files): 216 candidate prototypes, 0 flagged.
  • Every one of the 438 GTest TEST (suite, case) macros across tests/capi/*.cc (these get tagged as ordinary functions by ctags' generic C parser) is immediately preceded by a comment block containing @brief, verified with a separate direct script, not just the heuristic above.
  • Every named-tag struct/class in the tree (_ml_pipeline, _ml_pipeline_element, _ml_pipeline_common_elem, _type_int, _type_string, _type_element, _conf_detail, _camera_conf) has its own leading @brief.
  • This matches the PR's own CI: the self-test step ran with the real exuberant-ctags the "Preparing step 2" installs, and all 18 PASS: lines plus test_doxygen_tag.sh: all checks passed. appear in the live log of run 33715005844.

I did not find a counterexample. The claim holds as far as I can verify it.

Self-test / CI wiring

test_doxygen_tag.sh (18 fixtures) is a solid regression suite: it pins the negative control (undocumented definition must fail — the exact property that was silently broken for years), both scope halves (f vs f+p), all three /**< placements plus the two adjacency traps found in the origin review (trailing_belongs_to_next.hh, pointer_line.h/wrapped_pointer.h), the per-file state reset in both directions, and asserts zero stderr output (which is how the original local bug and the unset report_path actually manifested). It generates fixtures into mktemp -d rather than committing them, so an intentionally-undocumented fixture never lands in a future PR's $changed_file_list.

Positive note on the port, not an issue: in nnstreamer's static.check.yml this same self-test step's comment says "needs the same merge-ref guard as the self-test steps below" — there it's one of five sibling self-test steps sharing the pattern. nnstreamer/api's static.check.yml has no other self-test step, and the author rewrote the comment here to be self-contained (explaining why the merge-ref-vs-checkout-ref mismatch can happen, rather than pointing at nonexistent siblings). I checked — there is genuinely no other diff-filter=D/self-test step anywhere in this repo's .github/workflows/, so the rewritten comment is the correct call, not an oversight.

should-fix (not blocking this PR — outside its diff, but answers a question this review was asked to check)

nnstreamer/api's branch protection on main does not include "Static checks" as a required status check (gh api repos/nnstreamer/api/branches/main/protectionrequired_status_checks.contexts: ["DCO"] only). This predates this PR — the "Static checks" job already existed with a dozen other checkers before this change — and it isn't something this PR's diff can fix (the repo has no .github/settings.yml or other repo-as-code branch-protection file; it's a GitHub UI/API-only setting). But it directly bears on the review question "can CI failing actually block a future regression from merging": today, a red Static checks job (including a future regression this exact self-test would catch) does not by itself block the merge button, only a red/missing DCO does. Recommend the maintainer add Static checks to the required status checks for main in a follow-up (not part of this PR).

Informational / no PR action needed

  • Spell Check with Typos is currently failing on this PR's CI, but on a pre-existing typo ("gievn" → "given") in c/src/ml-api-service-offloading.c:306, introduced in commit 751f51b6 (2023-06-30), a file this PR does not touch. Confirmed via git blame against upstream/main. Unrelated to this change.
  • The known, already-accepted-upstream heuristic limits of has_trailing_doc/in_comment (can't distinguish a real /* from one inside a string literal or a x / *p expression without a space) carry over unchanged from the already-reviewed nnstreamer version; no live instance of either pattern exists in this tree today (checked by grep), same conclusion the origin reviews reached for nnstreamer's tree.
  • has_trailing_doc accepts any /**< without requiring @brief/@copydoc inside it, which is a slightly lower bar than the leading-comment form — again unchanged from the reviewed-and-approved upstream version, and consistent with how this repo's own headers already use /**< on enum/struct members.

Size / scope

Proportionate and minimal: 3 files, all under .github/workflows/, no source file touched (the tree already being clean under the corrected scope means there's no comment-only backlog-clearing commit needed here, unlike the origin PR's 34 files). No unrelated module is touched. Single commit, DCO-signed, subject line follows [Component] Summary convention, body explains what/why and credits the upstream fix.

Architecture / API docs

None needed. CI-script-only change; this repo has no CLAUDE.md/AGENTS.md/Documentation/ referencing the doxygen-check policy that this PR's scope choices would need to update.

CI status at review time

Static checks: pass (run 33715005844), self-test 18/18 PASS, doxygen-tag check itself ran clean over the 2 changed files. DCO: pass. Spell Check with Typos: fail, pre-existing/unrelated (see above). build (armeabi-v7a): pass. Remaining build (*)/Tizen GBS jobs were still pending/queued at review time; this PR touches no source or build file, so I have no reason to expect them to be affected, but I did not wait for them to finish.

Verdict

Mergeable 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.

@myungjoo
myungjoo marked this pull request as ready for review September 3, 2026 05:06
jaeyun-jung pushed a commit that referenced this pull request Sep 3, 2026
"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>
Sign up for freeto 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.

1 participant

@myungjoo
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Universal Dark Mode - works on any site\n(function() {\n var enabled = true;\n \n function applyDarkMode() {\n if (!enabled) return;\n \n // Create style element if it doesn't exist\n var style = document.getElementById('universal-dark-mode-style');\n if (!style) {\n style = document.createElement('style');\n style.id = 'universal-dark-mode-style';\n document.head.appendChild(style);\n }\n \n // Dark mode CSS - inverts colors but preserves images/video\n style.textContent = '\n /* Invert everything except media */\n html {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #1a1a2e !important;\n }\n \n /* Restore images, videos, iframes, canvas */\n img, video, iframe, canvas, svg, picture, [style*=\"background-image\"] {\n filter: invert(1) hue-rotate(180deg) !important;\n }\n \n /* Preserve specific elements that should not be inverted */\n .no-dark-mode, .no-dark-mode *,\n [data-theme=\"light\"], [data-theme=\"light\"],\n .ace_editor, .ace_editor *,\n .CodeMirror, .CodeMirror *,\n .monaco-editor, .monaco-editor *,\n .markdown-body pre, .markdown-body pre *,\n .highlight, .highlight *,\n pre code, pre code * {\n filter: none !important;\n }\n \n /* Fix common UI elements */\n .modal, .popup, .dropdown-menu, .tooltip, .popover {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #2d2d44 !important;\n border-color: #444 !important;\n }\n \n /* Scrollbars */\n ::-webkit-scrollbar { background: #1a1a2e !important; }\n ::-webkit-scrollbar-thumb { background: #444 !important; }\n ::-webkit-scrollbar-thumb:hover { background: #555 !important; }\n \n /* Selection */\n ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ';\n }\n \n function removeDarkMode() {\n var style = document.getElementById('universal-dark-mode-style');\n if (style) style.remove();\n }\n \n // Toggle with Alt+Shift+D\n document.addEventListener('keydown', function(e) {\n if (e.altKey && e.shiftKey && e.key === 'D') {\n e.preventDefault();\n enabled = !enabled;\n if (enabled) {\n applyDarkMode();\n console.log('[Universal Dark Mode] Enabled');\n } else {\n removeDarkMode();\n console.log('[Universal Dark Mode] Disabled');\n }\n }\n });\n \n // Apply on load\n applyDarkMode();\n \n // Re-apply on dynamic content\n var observer = new MutationObserver(function(mutations) {\n if (enabled && !document.getElementById('universal-dark-mode-style')) {\n applyDarkMode();\n }\n });\n observer.observe(document.head, { childList: true });\n \n console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');\n})();", "Universal Dark Mode"); } } catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); } })(); })();
Skip to content

[CI] Restore the per-function Doxygen check - #688

Open
myungjoo wants to merge 1 commit into
nnstreamer:mainfrom
myungjoo:ci/doxygen-function-check
Open

[CI] Restore the per-function Doxygen check#688
myungjoo wants to merge 1 commit into
nnstreamer:mainfrom
myungjoo:ci/doxygen-function-check

Conversation

@myungjoo

Copy link
Copy Markdown
Member

Port of nnstreamer/nnstreamer#4914 (issue nnstreamer/nnstreamer#4908).

What was wrong

.github/workflows/static.check.scripts/doxygen-tag.sh here is byte-identical to the nnstreamer copy: local function_check_flag="f+p" at script top level, which bash rejects, and no set -e, so ctags -x --c-kinds= ran with an empty kind list and the per-function @brief check has passed vacuously since the Actions import. $report_path was never set (an "ambiguous redirect" per file on stderr) and $brief was 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.yml with the same merge-ref guard:

  • definitions are checked everywhere, prototypes only in headers;
  • the trailing /**< ... */ form is recognised (declaration line, a line opening with /**< right after it, or after an inline body);
  • block comments are tracked so a * at the start of a wrapped parameter line is not taken for a comment continuation;
  • report_path defaults to /dev/null, brief resets per file;
  • test_doxygen_tag.sh generates 18 fixtures into a temp dir; all 18 fail against the checker on main and 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

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>
@myungjoo

Copy link
Copy Markdown
MemberAuthor

[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 (gh pr view 4914 --repo nnstreamer/nnstreamer --comments, 4 review rounds), diffed this PR (upstream/main...HEAD, 3 files, +325/-8) and byte-compared doxygen-tag.sh/test_doxygen_tag.sh against nnstreamer's final approved commit 4490a668 (identical modulo CRLF), read static.check.yml's new step and the rest of the job, and independently re-verified the PR's "zero backlog" claim against this repo's own 48 C/C++ files (ctags is not available in my sandbox, so I wrote and sanity-checked a Python heuristic against the checker's actual state-machine rules, cross-checked with gh pr checks / gh run view --log). Also checked nnstreamer/api's branch protection via the GitHub API.

Does it solve the stated problem

Yes. doxygen-tag.sh here was byte-identical to nnstreamer's pre-#4914 copy, carrying the same top-level local function_check_flag="f+p" (invalid outside a function, silently emptying ctags's kind list with no set -e to catch it), the same unset $report_path, and the same missing per-file reset of $brief. This PR takes nnstreamer's already-fixed, 4-round-reviewed version verbatim — I confirmed diff -u --strip-trailing-cr between this PR's two scripts and nnstreamer PR #4914's head (4490a668) is empty. That version's design (definitions checked everywhere, prototypes only in .h/.hh/.hpp; trailing /**< recognition on same line, next line, or after an inline body; real block-comment tracking via in_comment instead of a bare-* heuristic) was independently traced and stress-tested across 4 review rounds on the origin PR, including two real counterexamples found and fixed in-tree (gsttensor_converter.c's wrapped-pointer prototype, Looper::post getting credited for a sibling's trailing /**<). Nothing in this port reopens either of those.

Backlog = 0 claim — verified independently

I could not run ctags locally (not installed, no sudo in my sandbox), so instead of trusting the PR body I wrote a Python approximation of the checker's own rules (K&R return-type-on-own-line handling, @brief/@copydoc block detection, trailing /**< on the same/next line) and sanity-checked it against a synthetic documented/undocumented pair before trusting its output on the real tree.

  • Whole-tree scan (48 files): 1141 candidate function definition/declaration sites, 0 flagged as undocumented.
  • Header-prototype-only scan (f+p scope, 9 header files): 216 candidate prototypes, 0 flagged.
  • Every one of the 438 GTest TEST (suite, case) macros across tests/capi/*.cc (these get tagged as ordinary functions by ctags' generic C parser) is immediately preceded by a comment block containing @brief, verified with a separate direct script, not just the heuristic above.
  • Every named-tag struct/class in the tree (_ml_pipeline, _ml_pipeline_element, _ml_pipeline_common_elem, _type_int, _type_string, _type_element, _conf_detail, _camera_conf) has its own leading @brief.
  • This matches the PR's own CI: the self-test step ran with the real exuberant-ctags the "Preparing step 2" installs, and all 18 PASS: lines plus test_doxygen_tag.sh: all checks passed. appear in the live log of run 33715005844.

I did not find a counterexample. The claim holds as far as I can verify it.

Self-test / CI wiring

test_doxygen_tag.sh (18 fixtures) is a solid regression suite: it pins the negative control (undocumented definition must fail — the exact property that was silently broken for years), both scope halves (f vs f+p), all three /**< placements plus the two adjacency traps found in the origin review (trailing_belongs_to_next.hh, pointer_line.h/wrapped_pointer.h), the per-file state reset in both directions, and asserts zero stderr output (which is how the original local bug and the unset report_path actually manifested). It generates fixtures into mktemp -d rather than committing them, so an intentionally-undocumented fixture never lands in a future PR's $changed_file_list.

Positive note on the port, not an issue: in nnstreamer's static.check.yml this same self-test step's comment says "needs the same merge-ref guard as the self-test steps below" — there it's one of five sibling self-test steps sharing the pattern. nnstreamer/api's static.check.yml has no other self-test step, and the author rewrote the comment here to be self-contained (explaining why the merge-ref-vs-checkout-ref mismatch can happen, rather than pointing at nonexistent siblings). I checked — there is genuinely no other diff-filter=D/self-test step anywhere in this repo's .github/workflows/, so the rewritten comment is the correct call, not an oversight.

should-fix (not blocking this PR — outside its diff, but answers a question this review was asked to check)

nnstreamer/api's branch protection on main does not include "Static checks" as a required status check (gh api repos/nnstreamer/api/branches/main/protectionrequired_status_checks.contexts: ["DCO"] only). This predates this PR — the "Static checks" job already existed with a dozen other checkers before this change — and it isn't something this PR's diff can fix (the repo has no .github/settings.yml or other repo-as-code branch-protection file; it's a GitHub UI/API-only setting). But it directly bears on the review question "can CI failing actually block a future regression from merging": today, a red Static checks job (including a future regression this exact self-test would catch) does not by itself block the merge button, only a red/missing DCO does. Recommend the maintainer add Static checks to the required status checks for main in a follow-up (not part of this PR).

Informational / no PR action needed

  • Spell Check with Typos is currently failing on this PR's CI, but on a pre-existing typo ("gievn" → "given") in c/src/ml-api-service-offloading.c:306, introduced in commit 751f51b6 (2023-06-30), a file this PR does not touch. Confirmed via git blame against upstream/main. Unrelated to this change.
  • The known, already-accepted-upstream heuristic limits of has_trailing_doc/in_comment (can't distinguish a real /* from one inside a string literal or a x / *p expression without a space) carry over unchanged from the already-reviewed nnstreamer version; no live instance of either pattern exists in this tree today (checked by grep), same conclusion the origin reviews reached for nnstreamer's tree.
  • has_trailing_doc accepts any /**< without requiring @brief/@copydoc inside it, which is a slightly lower bar than the leading-comment form — again unchanged from the reviewed-and-approved upstream version, and consistent with how this repo's own headers already use /**< on enum/struct members.

Size / scope

Proportionate and minimal: 3 files, all under .github/workflows/, no source file touched (the tree already being clean under the corrected scope means there's no comment-only backlog-clearing commit needed here, unlike the origin PR's 34 files). No unrelated module is touched. Single commit, DCO-signed, subject line follows [Component] Summary convention, body explains what/why and credits the upstream fix.

Architecture / API docs

None needed. CI-script-only change; this repo has no CLAUDE.md/AGENTS.md/Documentation/ referencing the doxygen-check policy that this PR's scope choices would need to update.

CI status at review time

Static checks: pass (run 33715005844), self-test 18/18 PASS, doxygen-tag check itself ran clean over the 2 changed files. DCO: pass. Spell Check with Typos: fail, pre-existing/unrelated (see above). build (armeabi-v7a): pass. Remaining build (*)/Tizen GBS jobs were still pending/queued at review time; this PR touches no source or build file, so I have no reason to expect them to be affected, but I did not wait for them to finish.

Verdict

Mergeable 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.

@myungjoo
myungjoo marked this pull request as ready for review September 3, 2026 05:06
jaeyun-jung pushed a commit that referenced this pull request Sep 3, 2026
"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>
Sign up for freeto 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.

1 participant

@myungjoo