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

Track filenames parsed #150

Closed
wants to merge 13 commits into from
Closed

Conversation

xonixx
Copy link
Contributor

@xonixx xonixx commented Sep 20, 2022

This is the other PR in context of #144.

Here I tried to address the item 1. of your list @benhoyt.

Eventually I've found semi-elegant solution of how to simplify the position-to-file tracking.

So the idea is (using ast.Walk() functionality that we now have) to keep mapping of Node -> filename for each AST node during parsing individual files. Thus we can resolve the error position back to the file by relying on the node where the error happens.
This approach also allowed to get rid of the errorFileLine function, as expected.

The solution is almost final and test suite is passing. I'm working on final refactorings but would like you to take a look @benhoyt.

@xonixx xonixx mentioned this pull request Sep 20, 2022
@xonixx
Copy link
Contributor Author

xonixx commented Sep 20, 2022

Ok, I think I'm done.

@benhoyt
Copy link
Owner

benhoyt commented Sep 21, 2022

Hi @xonixx, sorry for not replying sooner. This approach is along the lines of what I was thinking originally. However, on reflection it's quite invasive into the existing parsing code and adds exported API surface that I'm not entirely sure about. Plus, it uses a big map of nodes, which is potentially a decent amount of memory usage for larger programs -- ideally we'd only have one item tracked per file.

I was mulling this over and playing with an approach for this the other day, and here's what I came up with: https://github.com/benhoyt/goawk/pull/151/files

It's an internal package for a start, so we don't export the API and we don't have to get the API exactly right the first time. The new type is FileReader, on which you call AddFile for every file you add, and it reads the file and tracks the path name and number of lines in each file (one item tracked per file, so tiny memory footprint). Then you call FileLine with the overall line number and it returns the file path and file-relative line number.

So it gets rid of errorFileLine in the same way, and hopefully can also be used for coverage file paths/line numbers.

I'm not sure I love the name FileReader, but I'm open to suggestions there.

Thoughts?

@benhoyt
Copy link
Owner

benhoyt commented Sep 21, 2022

Note that my PR isn't intended for merging as is. I haven't gone over it in details, and it needs comments and tests. I'm happy for you to tidy it up and get it ready for merging if you'd like. We just need to make sure the approach will work for your upcoming coverage handling as well.

@xonixx
Copy link
Contributor Author

xonixx commented Sep 21, 2022

Thanks, this looks simpler indeed. I’ll work on it.

@xonixx xonixx closed this Sep 22, 2022
@xonixx xonixx mentioned this pull request Sep 22, 2022
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.

2 participants