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.

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

Fix position of comments #297

Closed
wants to merge 2 commits into from
Closed

Conversation

oraluben
Copy link
Contributor

closes #294

if p1 < p2 {
return p1
} else {
return p1
Copy link
Collaborator

Choose a reason for hiding this comment

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

This always returns p1.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks! Looks like this is not really related since it's not breaking compilation.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Which also means we don’t need min at all—instead of fixing min, you could have replaced it in all places with its first arg. That makes me a bit nervous that maybe we still don’t fully understand the fix that is needed. I’d love to see some tests, or at the least a bit of explanation somewhere. Also, why the constant 1? (Why not 0? Why not some other nearby position?) Again, tests or docs would be great.

Copy link
Contributor Author

@oraluben oraluben Sep 3, 2020

Choose a reason for hiding this comment

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

You're right. So far this pr is only a POC for my current fuzzing project. I neither fully understand this but it seems to be working (or more lucky, correct) and I will try to understand and refine it later.

I use the const 1 just for simplicity, 0 didn't work for it's a special value token.NoPos = 0 in go.

Copy link
Collaborator

@josharian josharian Sep 3, 2020

Choose a reason for hiding this comment

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

Sounds good. I really appreciate you taking the time to investigate it. I hope go-fuzz is yielding good results for you.

@dvyukov
Copy link
Owner

dvyukov commented Nov 14, 2020

Can this be closed now?

@josharian
Copy link
Collaborator

Superseded by #306

@josharian josharian closed this Nov 14, 2020
@oraluben oraluben deleted the fix-comment branch April 29, 2021 07:17
Sign up for free to 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.

go-fuzz-build fails with Go 1.15 due to uncertain position of comments
3 participants