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

Comment and String locations #52

Merged
merged 2 commits into from
Aug 25, 2017
Merged

Comment and String locations #52

merged 2 commits into from
Aug 25, 2017

Conversation

schloerke
Copy link
Contributor

@schloerke schloerke commented May 24, 2017

Hello,

The changes in this PR fix the issues in #51 .

  • Lines were not tracked when comments were reached
  • STRING_STATE routine did not set the *yylloc to yyextra->loc, similar to other tokens

Best,
Barret

lexer.cpp Outdated
@@ -1,15 +1,15 @@
#line 1 "lexer.cpp"
#line 2 "/Users/barret/_/git/R/libgraphqlparser/libgraphqlparser/lexer.cpp"
Copy link
Contributor

Choose a reason for hiding this comment

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

this needs to be fixed up. I think cmake is running flex incorrectly; I just run it manually for commit.

@schloerke
Copy link
Contributor Author

@swolchok Cleaned up the files with the correct flex version. (Much cleaner PR)

@swolchok
Copy link
Contributor

Sorry this took me so long to get to! Had a baby in April & thusly have been super busy.

This looks mergeable to me. Worst case, the offsets are wrong in a different way, which doesn't leave us worse off. :)

Can you clean up the commit history? Up to you whether to squash it into a single commit or a couple commits, but we shouldn't end up with "undo whitespace", "remove personal paths", etc. in our history.

@schloerke
Copy link
Contributor Author

Congratulations!

I have force updated the PR. It is much cleaner now.

@swolchok swolchok merged commit 41c6d19 into graphql:master Aug 25, 2017
@swolchok
Copy link
Contributor

Thanks, that's really easy to follow.

@swolchok swolchok mentioned this pull request Aug 25, 2017
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.

3 participants