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 statement strings #4878

Merged
merged 6 commits into from
Nov 24, 2015
Merged

Fix statement strings #4878

merged 6 commits into from
Nov 24, 2015

Conversation

mark-rushakoff
Copy link
Contributor

This PR mostly adds a lot of missing QuoteIdent calls, but there were some other small changes to ensure that a statement can be converted to a string and re-parsed without differing from the original parse result.

@@ -510,7 +510,7 @@ func (p *Parser) parseUInt64() (uint64, error) {
// This function assumes the DURATION token has already been consumed.
func (p *Parser) parseDuration() (time.Duration, error) {
tok, pos, lit := p.scanIgnoreWhitespace()
if tok != DURATION_VAL && tok != INF {
if tok != DURATION_VAL && tok != INF && tok != NUMBER {
Copy link
Contributor

Choose a reason for hiding this comment

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

What is going on here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

FormatDuration does not print units if it has to print the duration in microseconds.

When that happens, if we parse the statement again, the token parses as a NUMBER instead of a DURATION_VAL.

Now that you've pointed it out and now that I've read the EBNF closer, I see that I should not have changed parseDuration but I should have changed FormatDuration to include units because a duration_lit must have units. Not sure why I thought the unit was optional on a duration_lit – good catch. I'll remove the commit that includes this line, and I'll add a new commit that ensures FormatDuration prints units in the case of microseconds.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, as I'm digging into this one, I'm seeing conflicting information between the EBNF and what the code does and what the comments say the code should do.

I'm going to remove the commit from this PR and open a separate issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I removed the commit that modified parseDuration from this PR. Good eye, @otoolep. I was confused by the inconsistency with FormatDuration not printing units when the duration had to be represented in microseconds. That fix is in #4891.

@otoolep
Copy link
Contributor

otoolep commented Nov 24, 2015

+1 from me, but this really needs @dgnorton too.

@dgnorton
Copy link
Contributor

+1

mark-rushakoff added a commit that referenced this pull request Nov 24, 2015
@mark-rushakoff mark-rushakoff merged commit 28df5a1 into master Nov 24, 2015
@mark-rushakoff mark-rushakoff deleted the fix-statement-strings branch November 24, 2015 20:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants