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

wip allow for op suffixes #117

Closed
wants to merge 3 commits into from
Closed

wip allow for op suffixes #117

wants to merge 3 commits into from

Conversation

KristofferC
Copy link
Member

@KristofferC KristofferC commented Sep 20, 2017

@KristofferC
Copy link
Member Author

KristofferC commented Sep 20, 2017

@ZacLN As discussed with @Keno, there are two ways to handle this.

One is either as in this PR, to have a custom "operator suffix" kind (I called it badly SPEC_OP here but we can fix name later) that we emit as a separate token after the standard operator.
The other is to effectively double the number of kinds such that each operator has an associated special operator. I think I like the way done in this PR more because it reduces the number of kinds and the code churn here, but perhaps there is a difference in how well it translates to the parsing code.

@ZacLN
Copy link
Collaborator

ZacLN commented Sep 20, 2017

My intuition is that this is the better approach

@codecov-io
Copy link

codecov-io commented Sep 20, 2017

Codecov Report

Merging #117 into master will decrease coverage by 0.65%.
The diff coverage is 30.76%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #117      +/-   ##
==========================================
- Coverage   76.15%   75.49%   -0.66%     
==========================================
  Files           4        4              
  Lines         847      857      +10     
==========================================
+ Hits          645      647       +2     
- Misses        202      210       +8
Impacted Files Coverage Δ
src/token.jl 73.77% <100%> (ø) ⬆️
src/lexer.jl 85.54% <25%> (-1.02%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b3c8238...952f882. Read the comment docs.

@Keno
Copy link
Member

Keno commented May 22, 2018

@ZacLN @KristofferC Are we good with this approach? If so, shall we merge?

@ZacLN
Copy link
Collaborator

ZacLN commented May 22, 2018

Can I have a think on how to implement it in cstparser first? I'll have a look this weekend

@Keno
Copy link
Member

Keno commented May 30, 2018

Bump!

@ZacLN
Copy link
Collaborator

ZacLN commented Jun 2, 2018

Sorry for delay.

Thoughts- this doesn't do the job properly. There's no ambiguity in these cases and no reliance on context (above what is known at the Tokenize level) so we should be returning a single Token.

Similarly, do the changes in v0.7 make use of dot operators unambiguous? i.e. should they be handled to return a single TOken at this level?

EDIT: Further thoughts: introduce an extra function call before emitting an operator that checks for suffixes and appends it to the val field + possibly add a Boolean field to Token/RawToken indicating its not a 'pure' operator?

@ZacLN
Copy link
Collaborator

ZacLN commented Jun 3, 2018

An example for dotted operator handling handling
#128

@Keno
Copy link
Member

Keno commented Jun 18, 2018

Superseded by #129

@Keno Keno closed this Jun 18, 2018
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.

4 participants