goawk icon indicating copy to clipboard operation
goawk copied to clipboard

Track filenames parsed

Open xonixx opened this issue 2 years ago • 3 comments

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 avatar Sep 20 '22 12:09 xonixx

Ok, I think I'm done.

xonixx avatar Sep 20 '22 17:09 xonixx

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 avatar Sep 21 '22 08:09 benhoyt

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.

benhoyt avatar Sep 21 '22 08:09 benhoyt

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

xonixx avatar Sep 21 '22 21:09 xonixx