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

Report reader errors at the start of token #1255

Merged
merged 2 commits into from Apr 17, 2021

Conversation

yuhan0
Copy link
Contributor

@yuhan0 yuhan0 commented Apr 17, 2021

This allows editor plugins to place the squiggly lines below the entire token instead of right at the end.

@yuhan0
Copy link
Contributor Author

yuhan0 commented Apr 17, 2021

Note: this causes a 1% slowdown in performance, probably because parse-token is now extracting the row and column on every token instead of on exception only.
I tried expanding out the juxtinto separate bindings, but that made no difference when testing locally.

@borkdude
Copy link
Member

I think adding the row and col in a separate binding makes sense as it saves the overhead of juxt + creating a vector.
Have you tested performance on various big Clojure files?

@yuhan0
Copy link
Contributor Author

yuhan0 commented Apr 17, 2021

Have you tested performance on various big Clojure files?

I'm not sure how to do that, I've only run the script/diff which I think compares the performance on several big repos with the master branch.
The results (before and after refactoring juxt)

< linting took 17813ms, errors: 264, warnings: 2292
---
> linting took 17718ms, errors: 264, warnings: 2292
< linting took 17678ms, errors: 264, warnings: 2292
---
> linting took 16590ms, errors: 264, warnings: 2292

@borkdude
Copy link
Member

@yuhan0 You could try to download a binary from the below builds (CircleCI under artifacts) and then run the binary over clj-kondo itself. This takes around 1100ms on my machine with the most recent release. I will also test with your branch version.

@borkdude
Copy link
Member

@yuhan0 Oh, we skip the macOS build on branches. So you would have to build yourself locally.

@borkdude
Copy link
Member

I don't see a significant difference:

borkdude@MBP2019 ~/Dropbox/dev/clojure/clj-kondo (yuhan0-fix-reader-error-loc) $ /usr/local/bin/clj-kondo --lint src:test
linting took 1122ms, errors: 0, warnings: 0
borkdude@MBP2019 ~/Dropbox/dev/clojure/clj-kondo (yuhan0-fix-reader-error-loc) $ ./clj-kondo --lint src:test
linting took 1127ms, errors: 0, warnings: 0
borkdude@MBP2019 ~/Dropbox/dev/clojure/clj-kondo (yuhan0-fix-reader-error-loc) $ /usr/local/bin/clj-kondo --lint src:test
linting took 1135ms, errors: 0, warnings: 0
borkdude@MBP2019 ~/Dropbox/dev/clojure/clj-kondo (yuhan0-fix-reader-error-loc) $ ./clj-kondo --lint src:test
linting took 1150ms, errors: 0, warnings: 0

@borkdude borkdude merged commit ce0b5e2 into clj-kondo:master Apr 17, 2021
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.

None yet

2 participants