Skip to content
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.

Sign up for GitHub

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

Jump to bottom

handle some block comment to detect generated files #1161

Merged
merged 3 commits into from May 25, 2020
Merged

handle some block comment to detect generated files #1161

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
03e77af
handle some block comment to detect generated files
ksoichiro May 23, 2020
df8d6f9
add test for long line
ksoichiro May 23, 2020
8690f3a
parse file to find generated comment
ksoichiro May 23, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
23 changes: 22 additions & 1 deletion pkg/result/processors/autogenerated_exclude.go
View file
Open in desktop
Expand Up @@ -130,11 +130,32 @@ func getDoc(filePath string) (string, error) {
scanner.Buffer(make([]byte, initialSize), maxSize)

var docLines []string
inBlockComment := false
for scanner.Scan() {
line := strings.TrimSpace(scanner.Text())
if strings.HasPrefix(line, "//") {
if inBlockComment {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sorry, I don't think it's a good idea to manually parse comments.
Maybe we can use ast.ParseFile with flag like PackageClauseOnly or similar to stop early? I'm sure that performance won't be an issue here if we implement it properly.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for your advice.
I was actually on the fence about using AST, but I'll try to do that.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've changed to stop parsing manually by using parser.ParseFile().
Although parser.ParseFile() reads entire file regardless of mode, with PackageClauseOnly mode (and ParseComments mode), it stops parsing files after import statements, as you pointed out.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The text may appear anywhere in the file.

golang/go#13560 (comment)

Are we sure that avoiding possible performance regression has higher priority that compliance with convention?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that it looks more important. I guess time still will be <100ms total. We can easily check it: golangci-lint run -v shows all processors' times.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean compliance with convention is more important?

Suppose you think so.. performance regression depends on how many issues will be reported on generated files, so I think it's difficult to choose which repository to measure for this purpose. But as you explain in usage, parsing file seems not so much cost.

Parsing one source file takes 200 us on average. Parsing of all files in $GOROOT/src takes 2 seconds. Currently we parse each file more than once because it's not the bottleneck.

However, you also write like this:

We're planning to parse each file only once.

If we check entire file for comments, should we use existing AST instead of parsing file here again?
I haven't confirmed if it's possible yet, I guess we can do so since this function is called on reporting phase.

text := line
if strings.Contains(text, "*/") {
inBlockComment = false
text = text[:strings.Index(text, "*/")]
}
text = strings.TrimSpace(strings.TrimPrefix(text, "*"))
if text != "" {
docLines = append(docLines, text)
}
} else if strings.HasPrefix(line, "//") {
text := strings.TrimSpace(strings.TrimPrefix(line, "//"))
docLines = append(docLines, text)
} else if strings.HasPrefix(line, "/*") {
text := strings.TrimSpace(strings.TrimPrefix(line, "/*"))
if !strings.Contains(text, "*/") {
inBlockComment = true
} else {
text = strings.TrimSpace(text[:strings.Index(text, "*/")])
}
if text != "" {
docLines = append(docLines, text)
}
} else if line == "" || strings.HasPrefix(line, "package") {
// go to next line
} else {
Expand Down
9 changes: 9 additions & 0 deletions pkg/result/processors/autogenerated_exclude_test.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

16 changes: 16 additions & 0 deletions pkg/result/processors/testdata/autogen_exclude_block_comment.go
View file
Open in desktop
@@ -0,0 +1,16 @@
/*
* first line
*
* second line
* third line
*/

// and this text also

/*
this type of block comment also
*/

/* this one line comment also */

package testdata