Skip to content
This repository has been archived by the owner on Jun 9, 2023. It is now read-only.

Deprecation fixes and other updates #54

Merged
merged 5 commits into from
Jul 26, 2016
Merged

Deprecation fixes and other updates #54

merged 5 commits into from
Jul 26, 2016

Conversation

KristofferC
Copy link
Member

@KristofferC KristofferC commented Jul 21, 2016

Before:
Parser is ~64.83% slower than base

After:
Parser is ~13.38% slower than base

Parses correctly the same amount of files in Base as before and passes the same amount of tests.

Fixes #52

@@ -118,8 +118,6 @@ function sortedcomplement(of::SourceRange, set)
end

⨳(sym::Symbol) = Expr(sym)
⨳(sym::Symbol, args::LineNumberNode...) = Expr(sym::Symbol, args...)
Copy link
Member

@jakebolewski jakebolewski Jul 22, 2016

Choose a reason for hiding this comment

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

@KristofferC these may not be exercised in the tests but they may be used in the ASTInterpreter.jl source code, have you checked this?

Copy link
Member Author

Choose a reason for hiding this comment

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

It feels impossible to guard against breaking other packages that rely on unexported and untested methods but I can just add these back, no problems.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, they are put back now.

@jakebolewski
Copy link
Member

Nice! Glad to see that even with tracking the source information performance is still competitive.

@KristofferC
Copy link
Member Author

Do you think the type instability of Token has a major performance impact? One way of defining a Token could be with just a string field and a "Kind" field that says what type of Token it is. Kind would typically just be an Enum listing all the different Token types that exist.

@jakebolewski
Copy link
Member

Hmm, I wouldn't want to speculate on the performance diff before trying it first but seems worth trying and shouldn't be too hard.

This LGTM.

@jakebolewski jakebolewski merged commit b76e328 into master Jul 26, 2016
@KristofferC KristofferC deleted the kc/updates branch July 26, 2016 00:14
@ctzurcanu ctzurcanu mentioned this pull request May 11, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants