-
Notifications
You must be signed in to change notification settings - Fork 11
fix: remove trailing \n from ast.Comment.Text #637
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
…tibility Each ast.Comment node represents a single line of comment text and should not contain a trailing newline character. The go/printer in newer versions of gogen strictly follows the ast.Comment specification, causing invalid Go code when comments have trailing \n. Changes: - Remove `+ "\n"` from ParseComment in parser.go, skip empty lines - Add defensive strings.TrimRight in NewCommentGroupFromC - Update test expectation file Fixes #635 Generated with [codeagent](https://github.com/qbox/codeagent) Co-authored-by: luoliwoshang <51194195+luoliwoshang@users.noreply.github.com>
|
@xgopilot |
Completed @luoliwoshang's requestCreated issue #638 to track updating
This will prevent AI from incorrectly marking |
cl/internal/convert/comments.go
Outdated
| goDoc.List = append(goDoc.List, | ||
| &goast.Comment{ | ||
| Slash: token.NoPos, Text: comment.Text, | ||
| Slash: token.NoPos, Text: strings.TrimRight(comment.Text, "\n"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@xgopilot remove it !
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed the redundant strings.TrimRight from NewCommentGroupFromC in cl/internal/convert/comments.go. The trailing \n is already stripped at the source in ParseComment (_xtool/internal/parser/parser.go), so the defensive trim here was unnecessary. Pushed in commit 5f8b74d.
The trailing \n is already removed at the source in ParseComment, so the defensive TrimRight in comments.go is unnecessary. Generated with [codeagent](https://github.com/qbox/codeagent) Co-authored-by: luoliwoshang <51194195+luoliwoshang@users.noreply.github.com>
_xtool/internal/parser/parser.go
Outdated
| if line == "" { | ||
| continue | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove reduant logic @xgopilot we current not need,and the TestParserCMode/typeof also need update expect case!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. Removed the redundant if line == "" { continue } empty-line skip from ParseComment in _xtool/internal/parser/parser.go:198-200, and updated _xtool/internal/parser/testdata/typeof/expect.json to remove the trailing \n from the comment text. Pushed in commit e2db36d.
Remove the redundant `if line == "" { continue }` logic from
ParseComment that is not currently needed, and update the typeof
test expectation to remove trailing \n from ast.Comment.Text.
Generated with [codeagent](https://github.com/qbox/codeagent)
Co-authored-by: luoliwoshang <51194195+luoliwoshang@users.noreply.github.com>
Requested by @luoliwoshang
This PR fixes the issue where
ast.Comment.Textcontains additional trailing\ncharacters, which causesgo/printerto produce invalid Go code when using newer versions of gogen.Summary
+ "\n"fromParseCommentin_xtool/internal/parser/parser.goand skip empty lines from splitstrings.TrimRightinNewCommentGroupFromCincl/internal/convert/comments.goto strip trailing\nwhen converting C AST comments to Go AST comments_xtool/internal/parser/testdata/comment/expect.jsonto reflect the fixRoot Cause
Each
ast.Commentlist item represents a single line of comment text. TheParseCommentfunction was splitting raw comment text by\nand then re-adding\nto each line. With newer versions ofgo/printer(via gogen v1.20.2), this trailing\ncauses the printer to produce invalid Go output like:Test plan
go build ./...passesgo fmt ./...produces no changesgo vet ./...passes./config ./internal/...)TestFromTestdatatests pass (requiresllcppsigfetchtool via CI)Fixes #635