From 1222a6ba79a41f015e744164b3b02f6d55ef2b3d Mon Sep 17 00:00:00 2001 From: MyungJoo Ham Date: Thu, 3 Sep 2026 13:25:42 +0900 Subject: [PATCH] [CI] Restore the per-function Doxygen check 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 Co-Authored-By: Claude Fable 5.1 --- .../static.check.scripts/doxygen-tag.sh | 62 ++++- .../static.check.scripts/test_doxygen_tag.sh | 254 ++++++++++++++++++ .github/workflows/static.check.yml | 17 ++ 3 files changed, 325 insertions(+), 8 deletions(-) create mode 100644 .github/workflows/static.check.scripts/test_doxygen_tag.sh diff --git a/.github/workflows/static.check.scripts/doxygen-tag.sh b/.github/workflows/static.check.scripts/doxygen-tag.sh index d3f86964..244731dc 100755 --- a/.github/workflows/static.check.scripts/doxygen-tag.sh +++ b/.github/workflows/static.check.scripts/doxygen-tag.sh @@ -28,12 +28,42 @@ fi files=$1 failed=0 +report_path=${report_path:-/dev/null} if [ ! -f $files ]; then echo "::error The file $files does not exists." exit 1 fi +## +# @brief Tell whether the declaration starting at a line carries a trailing +# "/**<" Doxygen comment, on the declaration itself or right after it. +# @param $1 1-based line number where ctags placed the function +# @return 0 when such a comment is found, 1 otherwise +# Reads the global array file_lines (0-based). A declaration ends at the first +# ';' outside braces, or at the '}' that closes an inline body. The comment may +# also open the next line; a "/**<" later on that line belongs to whatever is +# declared there, not to this one. +has_trailing_doc() { + local -i i=$1-1 + local -i last=${#file_lines[@]}-1 + local -i depth=0 + local l opens closes + while [[ $i -le $last ]]; do + l=${file_lines[$i]} + [[ $l == *"/**<"* ]] && return 0 + opens=${l//[^\{]/} + closes=${l//[^\}]/} + depth+=${#opens}-${#closes} + if [[ $depth -le 0 && ( $l == *";"* || $l == *"}"* ) ]]; then + [[ $i -lt $last && ${file_lines[$i+1]} =~ ^[[:space:]]*/\*\*\< ]] && return 0 + return 1 + fi + i+=1 + done + return 1 +} + echo "::group::Doxygen tag check started" for file in `cat $files`; do @@ -69,14 +99,20 @@ for file in `cat $files`; do # Checking tags for each function if [[ $advanced == 1 ]]; then declare -i idx=0 + brief=0 + in_comment=0 function_positions="" # Line number of functions. structure_positions="" # Line number of structure. + mapfile -t file_lines < "$file" - local function_check_flag="f+p" # check document for function and prototype of the function - - if [[ $pr_doxygen_check_skip_function_definition == 1 && $file != *.h ]]; then - function_check_flag="p" # check document for only prototypes of the function for non-header file - fi + # Definitions are checked everywhere. Prototypes are checked + # only in headers, where the declaration is the documented + # form; a forward declaration in a .c/.cc is documented at + # its definition. + case $file in + *.h|*.hh|*.hpp ) function_check_flag="f+p" ;; + * ) function_check_flag="f" ;; + esac # Find line number of functions using ctags, and append them. while IFS='' read -r line || [[ -n "$line" ]]; do @@ -97,7 +133,7 @@ for file in `cat $files`; do # Check if a function has @brief tag or not. # To pass correct line number not sub number, keep space " $idx ". # ex) want to pass 143 not 14, 43, 1, 3, 4 - if [[ $function_positions =~ " $idx " && $brief -eq 0 ]]; then + if [[ $function_positions =~ " $idx " && $brief -eq 0 ]] && ! has_trailing_doc $idx; then echo "[ERROR] File name: $file, $idx line, `echo $line | cut -d ' ' -f1` function needs @brief tag " failed=1 fi @@ -113,11 +149,21 @@ for file in `cat $files`; do # Find brief or copydoc tag in the comments between the codes. if [[ $line =~ "@brief" || $line =~ "@copydoc" ]]; then brief=1 - # Doxygen tags become zero in code section. - elif [[ $line != *"*"* && ( $line =~ ";" || $line =~ "}" || $line =~ "#") ]]; then + # Doxygen tags become zero in code section. Lines inside a + # block comment, or opening one, keep the pending tag; a + # code line that merely contains '*' (a pointer) does not. + elif [[ $in_comment -eq 0 && ! $line =~ ^[[:space:]]*(/\*|//) && ( $line =~ ";" || $line =~ "}" || $line =~ "#") ]]; then brief=0 fi + # Track block comments so that their continuation lines are + # never mistaken for code, whatever character they start with. + if [[ $in_comment -eq 1 ]]; then + [[ $line == *"*/"* ]] && in_comment=0 + elif [[ $line == *"/*"* && ${line##*/\*} != *"*/"* ]]; then + in_comment=1 + fi + # Check a comment statement that begins with '/*'. # Note that doxygen does not recognize a comment statement that start with '/*'. # Let's skip the doxygen tag inspection such as "/**" in case of a single line comment. diff --git a/.github/workflows/static.check.scripts/test_doxygen_tag.sh b/.github/workflows/static.check.scripts/test_doxygen_tag.sh new file mode 100644 index 00000000..ee915f9d --- /dev/null +++ b/.github/workflows/static.check.scripts/test_doxygen_tag.sh @@ -0,0 +1,254 @@ +#!/usr/bin/env bash + +## +# Copyright (c) 2026 Samsung Electronics Co., Ltd. All Rights Reserved. +# +# @file test_doxygen_tag.sh +# @brief Self-test for doxygen-tag.sh in the advanced (per-function) mode. +# @see https://github.com/nnstreamer/nnstreamer +# @author MyungJoo Ham +# +# Runs doxygen-tag.sh the way static.check.yml does, with a changed-file list +# and the advanced flag. The per-function check had passed vacuously for two +# years because a stray "local" emptied the ctags kind list, so the first +# property pinned here is the negative control: an undocumented function +# definition must fail. The rest pin the scope of the check (definitions +# everywhere, prototypes only in headers), the trailing "/**<" form, and the +# per-file reset of the tag state. +# +# Fixtures are generated into a temporary directory rather than committed, so +# that the intentionally undocumented ones never appear in a changed-file list. + +set -u + +SCRIPT_DIR=$(cd "$(dirname "$0")" && pwd) +REPO_ROOT=$(cd "${SCRIPT_DIR}/../../.." && pwd) +CHECKER="${SCRIPT_DIR}/doxygen-tag.sh" +workdir=$(mktemp -d) +failed=0 + +trap 'rm -rf "$workdir"' EXIT + +if ! command -v ctags > /dev/null 2>&1; then + echo "::error ctags is required by doxygen-tag.sh but is not installed." + exit 1 +fi + +# File-level tags every C/C++ fixture needs so that only the per-function +# rules decide the outcome. The include line matters: the checker treats a +# @brief as pending until the next code line, and a real file always has one +# between the header comment and the first function. +header_block() { + printf '/**\n * @file %s\n * @brief fixture\n * @author fixture\n * @bug none\n */\n#include \n' "$1" +} + +# write_fixture +# Creates workdir/ with the standard file header followed by content. +write_fixture() { + local name=$1 content=$2 + { header_block "$name"; printf '%s\n' "$content"; } > "${workdir}/${name}" +} + +# run_checker ... +# Runs the checker on the listed fixtures in advanced mode, from the repo +# root, and compares the exit status. Also fails on any stderr output, which +# is how the broken "local" and the unset report_path used to show up. +run_checker() { + local expected=$1 desc=$2 + shift 2 + local list errfile actual f + + list=$(mktemp -p "$workdir") + errfile=$(mktemp -p "$workdir") + for f in "$@"; do + printf '%s\n' "${workdir}/${f}" >> "$list" + done + + (cd "$REPO_ROOT" && bash "$CHECKER" "$list" 1) > /dev/null 2> "$errfile" < /dev/null + actual=$? + [[ $actual -ne 0 ]] && actual=1 + + if [[ "$actual" != "$expected" ]]; then + echo "FAIL: ${desc} expected exit ${expected}, got ${actual}" + failed=1 + elif [[ -s "$errfile" ]]; then + echo "FAIL: ${desc} wrote to stderr:" + sed 's/^/ /' "$errfile" + failed=1 + else + echo "PASS: ${desc} (exit ${actual})" + fi +} + +write_fixture documented.c '/** + * @brief add one + */ +int +add_one (int x) +{ + return x + 1; +}' +run_checker 0 "documented definition passes" documented.c + +write_fixture undocumented.c 'int +add_one (int x) +{ + return x + 1; +}' +run_checker 1 "undocumented definition fails" undocumented.c + +write_fixture forward_decl.c 'static int add_one (int x); + +/** + * @brief add one + */ +static int +add_one (int x) +{ + return x + 1; +}' +run_checker 0 "undocumented forward declaration in a .c passes" forward_decl.c + +write_fixture undocumented_proto.h 'int add_one (int x);' +run_checker 1 "undocumented prototype in a .h fails" undocumented_proto.h + +write_fixture documented_proto.h '/** + * @brief add one + */ +int add_one (int x);' +run_checker 0 "documented prototype in a .h passes" documented_proto.h + +write_fixture trailing_same_line.hh '/** @brief adder */ +class adder +{ + public: + /** @brief the class */ + adder (); + int add_one (int x); /**< add one */ +};' +run_checker 0 "trailing /**< on the declaration line passes" trailing_same_line.hh + +write_fixture trailing_next_line.hh '/** @brief adder */ +class adder +{ + public: + /** @brief the class */ + adder (); + virtual int add_one (int x) = 0; + /**< add one */ + virtual int add_two (int x, + int y) = 0; + /**< add two, declared over two lines */ +};' +run_checker 0 "trailing /**< on the following line passes" trailing_next_line.hh + +write_fixture trailing_after_body.hh '/** @brief adder */ +class adder +{ + public: + /** @brief the class */ + adder (); + virtual int add_one (int x) + { + return x + 1; + } + /**< add one, documented after the inline body */ +};' +run_checker 0 "trailing /**< after an inline body passes" trailing_after_body.hh + +write_fixture trailing_missing.hh '/** @brief adder */ +class adder +{ + public: + /** @brief the class */ + adder (); + int add_one (int x); + int add_two (int x); + /**< documents add_two only */ +};' +run_checker 1 "declaration without any doc between documented ones fails" trailing_missing.hh + +write_fixture trailing_belongs_to_next.hh '/** @brief adder */ +class adder +{ + public: + /** @brief the class */ + adder (); + int add_one (int x); + int count; /**< documents count, not add_one */ +};' +run_checker 1 "a /**< on the next declaration does not cover this one" trailing_belongs_to_next.hh + +write_fixture undocumented_struct.h 'struct point +{ + int x; +};' +run_checker 1 "undocumented struct fails" undocumented_struct.h + +# The tag must not survive a code line just because that line has a '*' in +# it: a pointer parameter is not a comment. +write_fixture pointer_line.h '/** + * @brief add one in place + */ +void add_one (int *x); +void add_two (int *x);' +run_checker 1 "a brief does not carry over a pointer-typed declaration" pointer_line.h + +# A wrapped parameter list can put the '*' at the start of a line; that is +# still code, not a comment continuation. +write_fixture wrapped_pointer.h '/** + * @brief add one in place + */ +void add_one (int + *x); +void add_two (int *x);' +run_checker 1 "a brief does not carry over a wrapped pointer parameter" wrapped_pointer.h + +# The converse: a ';' inside the comment block must not clear the tag. +write_fixture semicolon_in_comment.h '/** + * @brief add one in place; the caller owns x + * usage: add_one (&x); + */ +void add_one (int *x);' +run_checker 0 "a semicolon inside the comment block keeps the brief" semicolon_in_comment.h + +write_fixture ends_in_brief.c '/** + * @brief add one + */ +int +add_one (int x) +{ + return x + 1; +} +/** + * @brief a dangling comment that leaves the tag state set + */' +run_checker 0 "file ending in a brief passes on its own" ends_in_brief.c + +# The file-level tags sit at the bottom here so that nothing resets the tag +# state before the first function; only the per-file reset can fail it. +{ printf 'int +add_one (int x) +{ + return x + 1; +} +'; header_block tail_header.c; } > "${workdir}/tail_header.c" +run_checker 1 "undocumented first function fails on its own" tail_header.c +run_checker 1 "tag state does not leak into the next file" ends_in_brief.c tail_header.c + +write_fixture no_author.c '/** + * @brief add one + */ +int +add_one (int x) +{ + return x + 1; +}' +sed -i '/@author/d' "${workdir}/no_author.c" +run_checker 1 "missing file-level @author fails" no_author.c + +if [[ "$failed" != "0" ]]; then + echo "::error test_doxygen_tag.sh failed." + exit 1 +fi +echo "test_doxygen_tag.sh: all checks passed." diff --git a/.github/workflows/static.check.yml b/.github/workflows/static.check.yml index 762ee1e2..9c897530 100644 --- a/.github/workflows/static.check.yml +++ b/.github/workflows/static.check.yml @@ -83,6 +83,23 @@ jobs: # Need "grep" run: | bash .github/workflows/static.check.scripts/doxygen-tag.sh $changed_file_list 1 + - name: /Checker/ doxygen-tag self-test + # Runs a repository script unrelated to $changed_file_list. The step + # list comes from the merge ref while the checkout above is the PR + # head, so a tree that lacks this script - whether the branch + # predates it or a rename/deletion left it out without updating this + # step - would fail with "No such file or directory". Skip only when + # this PR did not remove it (see the elif below). + run: | + selftest=.github/workflows/static.check.scripts/test_doxygen_tag.sh + if [ -f "$selftest" ]; then + bash "$selftest" + elif git show --pretty="format:" --name-only --no-renames --diff-filter=D ${{ github.event.pull_request.head.sha }} -${{ github.event.pull_request.commits }} | grep -qxF "$selftest"; then + echo "::error::This PR removes $selftest; remove its workflow step too, or restore the script (also update this step if the script moved)." + exit 1 + else + echo "Skipped: $selftest is not in this branch's tree and this PR does not remove it." + fi - name: /Checker/ Indent check # Originally from "pr-prebuild-indent" # Need "indent"