fix(cl): comments format bugfix - #933
Conversation
PR #928 enabled `-fparse-all-comments` unconditionally so that Python header documentation is preserved. That also surfaced formatting bugs in `toLineComments` for C/C++ comments, which produced garbled output and made TestClang fail. The marker stripping only handled a single, well-formed block. It left artifacts when: - a block opened with more than two asterisks ("/***" -> a stray "// *" line), as seen on clang_isPreprocessing / clang_isUnexposed; - a block closed with extra asterisks or was a banner of asterisks ("**/", "/*****...*****/"); - a raw comment concatenated several blocks (e.g. "/* Declarations */\n/** ... */", which libclang reports when a plain comment immediately precedes a doc comment on the same declaration), leaving interior "*/" and "/**" markers in the output. Rewrite the stripping to clean each physical line independently via a new `cleanCommentLine` helper that removes leading/trailing block markers (slash + run of "*"), line-comment markers ("//", "///", "//!") and a single Doxygen "*" decoration, wherever they appear. This cleans every case without changing any already-correctly-formatted comment (verified exhaustively against the clang-c and Python fixture headers). Update the two affected clang-c golden files (the stray "// *" lines from the "/***" openers) and add unit tests for `toLineComments`. The darwin-only gates in tool/gen_test.go are kept: the clang-c goldens remain platform-divergent (Apple Blocks typedefs and libclang-version comment association differ off-Apple), so enabling non-macOS would need separate goldens; the formatting fix itself is platform-independent.
![fennoai[bot]](https://avatars.githubusercontent.com/in/1924301?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: fix C/C++ doc comment formatting
The refactor to a line-by-line cleanCommentLine helper is a clean, well-reasoned fix. The doc comments explain why (libclang concatenating multiple comment blocks into one raw comment), the banner/triple-star/concatenation cases are handled correctly, and the new table-driven tests in cl/doc_test.go cover the real regressions (the stray // * lines removed from the two golden files).
Security and performance passes found nothing of concern: content can never become a Go directive (every non-empty line is emitted as "// " + line, and the space makes //go:///llgo: inert), and the code runs once per declaration on short input.
Two low-severity edge cases in the marker-stripping heuristic are noted inline. Neither blocks merge; they're robustness/documentation notes for inputs that libclang rarely emits.
A minor doc nit: the cleanCommentLine order sentence ("openers/closers first, then line-comment markers, then a single leading *") omits the middle switch case that blanks all-asterisk/banner lines — accurate but incomplete.
| if t := strings.TrimRight(s, "*"); len(t) < len(s) { | ||
| line = strings.TrimSpace(t) | ||
| } | ||
| } |
There was a problem hiding this comment.
The trailing-closer strip removes any *-run + / suffix from a line regardless of whether it is a true block closer or genuine content. A content line ending in **/ (e.g. a Doxygen line mentioning the glob pattern **/) is silently truncated: CutSuffix("/") → "...pattern **", then TrimRight(.,"*") → "...pattern ", dropping the **/.
The URL case (http://a/b/) is safe because no * precedes the final /, but this heuristic can't distinguish a terminator from content shaped like one. Rare in practice, but worth either guarding (only strip when the line is recognizably a terminator) or documenting the assumption in the function comment.
| if t := strings.TrimLeft(s, "*"); len(t) < len(s) { | ||
| line = strings.TrimSpace(t) | ||
| } | ||
| } |
There was a problem hiding this comment.
Symmetric to the trailing-closer case: a content line beginning with / followed by a run of * is treated as an opener and stripped, even when it's genuine content (e.g. prose starting with /*note*/ → /* stripped then */ stripped → note). The heuristic assumes the first/last */-shaped token on a line is always a marker, never content. Consider noting this assumption in the doc comment.
|
@fennoai The CI build is still failing on macOS. Since the |
macOS
|
1 participant
Requested by @xushiwei
Fixes the C/C++ doc comment formatting that PR #928 surfaced by always enabling
-fparse-all-comments.Problem
TestClangfailed because the generated doc comments contained garbled artifacts:// *line from/***openers (e.g. onclang_isPreprocessing/clang_isUnexposed);**// rows-of-asterisks remnants;// /**,// Declarations */) when libclang concatenates a plain/* ... */block with a following/** ... */doc block on the same declaration.Fix
toLineCommentsincl/doc.goto clean each physical line independently via a newcleanCommentLinehelper that strips leading/trailing block markers (/+ run of*, run of*+/), line-comment markers (//,///,//!), and a single Doxygen*decoration — wherever they appear, including interior positions.// *lines).toLineComments(/***, banners, multi-block, line comments, decoration).Comment extraction for both Clang and Python
TestPythonpasses unchanged — the regular-comment docs PR fix(tool): preserve Python header documentation #928 wanted are preserved and now formatted cleanly.TestClang,TestSingleC,TestPythonand the fullclsuite pass underllgo test(LLVM 22) locally.GOOS gates in
tool/gen_test.goLeft the
runtime.GOOS != "darwin"gates as-is. While investigating I ran the tests on Linux and confirmed the clang-c goldens are genuinely platform-divergent off-Apple (Apple Blocks typedefs become empty structs; libclang comment association differs by version), so enabling non-macOS would require separate goldens. The formatting fix itself is platform-independent, so macOS CI will produce exactly these updated goldens.