You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@thrift.apache.org by nsuke <gi...@git.apache.org> on 2016/01/06 12:31:22 UTC
[GitHub] thrift pull request: THRIFT-3528 Fix warnings in thrift.ll
GitHub user nsuke opened a pull request:
https://github.com/apache/thrift/pull/778
THRIFT-3528 Fix warnings in thrift.ll
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/nsuke/thrift THRIFT-3528
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/thrift/pull/778.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #778
----
commit 45a6d266ad8d7d4217b5b7779bc1b44bbccf680b
Author: Nobuaki Sukegawa <ns...@apache.org>
Date: 2016-01-06T11:30:07Z
THRIFT-3528 Fix warnings in thrift.ll
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request: THRIFT-3528 Fix warnings in thrift.ll
Posted by hcorg <gi...@git.apache.org>.
Github user hcorg commented on the pull request:
https://github.com/apache/thrift/pull/778#issuecomment-169698270
Comment is from older version, which for some reason passed all unexpected tokens to bison.. strange :)
So with comment removal this patch has my +1 :+1:
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request: THRIFT-3528 Fix warnings in thrift.ll
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/thrift/pull/778
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request: THRIFT-3528 Fix warnings in thrift.ll
Posted by Jens-G <gi...@git.apache.org>.
Github user Jens-G commented on the pull request:
https://github.com/apache/thrift/pull/778#issuecomment-169499891
Could you explain what is going on here and why these entries have to be changed/removed? I just don't get it, sorry.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request: THRIFT-3528 Fix warnings in thrift.ll
Posted by nsuke <gi...@git.apache.org>.
Github user nsuke commented on the pull request:
https://github.com/apache/thrift/pull/778#issuecomment-170418643
@hcorg oh I actually got your point, thanks for catching this. Comments are harder to read than C++ :p
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request: THRIFT-3528 Fix warnings in thrift.ll
Posted by nsuke <gi...@git.apache.org>.
Github user nsuke commented on the pull request:
https://github.com/apache/thrift/pull/778#issuecomment-169677418
Sure, that's the whole point of putting this as a PR.
In a nutshell I believe it does not change any behavior except that it fixes 3 warnings pasted to JIRA.
Those 3 rules are exact duplicates that never match because precedent ones (Line 182, 267 and 409) always match over these.
You may want to also check `git blame` output to see which patches caused these. It seems simple mistakes to me.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request: THRIFT-3528 Fix warnings in thrift.ll
Posted by hcorg <gi...@git.apache.org>.
Github user hcorg commented on the pull request:
https://github.com/apache/thrift/pull/778#issuecomment-169691258
thrift_reserved_keyword(yytext); is used to report "Cannot use reserved language keyword: " (reserved in Thrift target languages, not Thrift itself). As "union" is already used by Thrift, it does not need to be additionally reserved. "Public" is just repeated (looks like typo).
So for those changes: +1
But last one seems more complicated - at least looking at comment (I hate comments, but still, sometimes it's good to read them). I'm not sure which 'default' block should we keep. Why someone needed '*' in parser? Is it still needed? If we stay with "unexpected_token" (I'd prefer that) I think that comment should be killed too.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request: THRIFT-3528 Fix warnings in thrift.ll
Posted by nsuke <gi...@git.apache.org>.
Github user nsuke commented on the pull request:
https://github.com/apache/thrift/pull/778#issuecomment-169697734
235f8b5899b2c5b75cb83975695da1377f0cac0c is the commit that added current (preceding) one which is THRIFT-1274.
According to the JIRA ticket
> Currently, if the thrift lexer encounters a token it does not expect, it prints the token it to stdout and continues. (This is the default behavior of flex when a token is unmatched.) This updates thriftl.ll to fail with an error message when it sees an unexpected character.
So according to this, older one does nothing and we wanted Thrift compiler to fail on unexpected token, which seems perfectly reasonable to me.
Removing the comment makes sense. `unexpected_token` call is already more informative than the comment.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---