Some column numbers off when reading symbols with source-logging-push-back-reader

Description

I have not checked other variants of readers in tools.reader yet, but here are some steps to reproduce what I have found. Not big deals, just nice if the source code locations were not off by 1 in some cases. Search for "TBD" in the sample REPL session below to see the things that appear off to me.

Environment

None

Activity

Show:
Andy Fingerhut
December 1, 2014, 10:08 PM

The 0-indexed change means it is incompatible with the built-in Clojure reader in column numbers? That seems like a change that would make it difficult to use as a drop-in replacement.

I will try out the changes and let you know, after perhaps hacking in a local change to add 1 to all column numbers.

Nicola Mometto
December 1, 2014, 10:19 PM

I hadn't considered LineNumberingPushbackReader compatibility – I pushed another commit to the col-changes branch reverting to 1-indexed column numbers. I'll probably make a mailing list post asking for feedback on this change as I'm of the opinion that 0-indexed column numbers make more sense, but this is admittedly an enhancement out of scope for this bug report.

Andy Fingerhut
December 2, 2014, 12:17 AM

I tried out the changes with the 1-indexed column numbers, and in all of the test cases I have tried so far it seems to be doing what I expect, except perhaps not sure about comments with the source tracking reader. Here is an example, using the same read-with-locs function defined in the ticket description:

The line and column info is consistent with the location of the symbol 'foo'. The :source contains the preceding comment, and if there is more than one line containing a preceding comment, all of them, including intervening whitespace. That might be the only reasonable thing to do in this situation, but it seems worth mentioning, and perhaps documenting, if it is the expected behavior. Also adding a test case or 2 with comments, which I'd be happy to do if you like.

I have an updated file src/test/clojure/clojure/tools/metadata_test.clj with corrections to many :end-column values for the updated code, if that would be helpful.

Andy Fingerhut
December 2, 2014, 12:25 AM

Attached patch trdr-19-update-metadata-tests-v1.patch updates metadata tests for recent changes to :end-column.

I have checked all of the changes by hand and they are what I expect them to be.

Nicola Mometto
December 2, 2014, 12:46 AM

Thanks for the help, I just rebased my commits and pushed to master, including the patch you attached fixing the metadata test.
https://github.com/clojure/tools.reader/compare/52a95228cc...281b31a58c

The line/col info is orthogonal to the :source info of the source-logging readers and should be independent of comments/whitespaces so I'd say the current behaviour should be expected – agreed that some more documentation would be nice, I'll work on that.

Completed

Assignee

Nicola Mometto

Reporter

Andy Fingerhut

Labels

None

Approval

None

Patch

None

Priority

Major