trdr-6-eliminate-reflection-with-type-hints-patch-v1.txt
[TRDR-6] Some uses of reflection in tools.reader code slow it down unnecessarily Created: 13/Apr/13 Updated: 13/Apr/13 Resolved: 13/Apr/13 |
|
| Status: | Closed |
| Project: | tools.reader |
| Component/s: | None |
| Affects Version/s: | None |
| Fix Version/s: | None |
| Type: | Enhancement | Priority: | Minor |
| Reporter: | Andy Fingerhut | Assignee: | Nicola Mometto |
| Resolution: | Completed | Votes: | 0 |
| Labels: | None | ||
| Attachments: |
trdr-6-eliminate-reflection-with-type-hints-patch-v1.txt
|
| Patch: | Code |
| Description |
|
Attached patch uses type hints to eliminate several instances of reflection in the tools.reader code. FYI, you can run 'lein check' to cause Leiningen to compile the code with warn-on-reflection true, I believe for every source file (not sure about the tests, but reflection isn't such a big deal in them anyway). |
| Comments |
| Comment by Nicola Mometto [ 13/Apr/13 12:13 PM ] |
|
Andy, I don't see any patch attached to this ticket, I think you forgot to attach it. (P.S. thanks, I didn't know about 'lein check') |
| Comment by Andy Fingerhut [ 13/Apr/13 12:19 PM ] |
|
Patch trdr-6-eliminate-reflection-with-type-hints-patch-v1.txt dated Apr 13 2013 eliminates all occurrences of reflection found in latest version of tools.reader. Please check them carefully before committing them, especially the ones in default_data_readers.clj. And I know the reflection warnings in default_data_readers.clj exist in Clojure's code, too, where you copied those from. CLJ-1080 has a patch addressing those and many other reflections within Clojure's code. |
| Comment by Nicola Mometto [ 13/Apr/13 12:59 PM ] |
|
Andy, casting to char makes tools.reader crash under clojure-1.3, apparently casting to char is possible only from clojure 1.4. Could you please submit a patch removing reflection in default_data_readers.clj and the docstring fixes only while I try to figure out a way to maintain clojure 1.3 compatibility and remove the reflection? (or if you have an idea on how to do it, you're more than welcome EDIT: |
| Comment by Andy Fingerhut [ 13/Apr/13 2:43 PM ] |
|
Great. I was off-line there for a while, but glad you noticed the 1.3 compatibility issue where I didn't, and glad you found a different way to eliminate reflection with 1.4 and later. |
[TRDR-5] Additional "Differences from LispReader.java" for README Created: 08/Apr/13 Updated: 10/Apr/13 Resolved: 10/Apr/13 |
|
| Status: | Closed |
| Project: | tools.reader |
| Component/s: | None |
| Affects Version/s: | None |
| Fix Version/s: | None |
| Type: | Enhancement | Priority: | Minor |
| Reporter: | Andy Fingerhut | Assignee: | Nicola Mometto |
| Resolution: | Completed | Votes: | 0 |
| Labels: | None | ||
| Description |
|
Suggested addition to "Differences from LispReader.java" section of README.md: read is capable of reading the symbol / with an explicit namespace, e.g. foo//, whereas clojure.lang.LispReader/read throws an exception. Refer to CLJ-873. Except for this special case, read throws an exception if a symbol contains more than one / character, whereas clojure.lang.LispReader/read allows them, returning a symbol with one or more / characters in its namespace name. |
| Comments |
| Comment by Nicola Mometto [ 10/Apr/13 5:20 AM ] |
|
Thanks, fixed https://github.com/clojure/tools.reader/commit/f689cb283d1fb539a6cabbefd4036f620dabe693 |
[TRDR-3] Make read-line available in clojure.tools.reader and/or clojure.tools.reader.edn namespace? Created: 15/Mar/13 Updated: 15/Mar/13 Resolved: 15/Mar/13 |
|
| Status: | Closed |
| Project: | tools.reader |
| Component/s: | None |
| Affects Version/s: | None |
| Fix Version/s: | None |
| Type: | Enhancement | Priority: | Minor |
| Reporter: | Andy Fingerhut | Assignee: | Nicola Mometto |
| Resolution: | Completed | Votes: | 0 |
| Labels: | None | ||
| Description |
|
You mention the enhanced version of read-line that takes a reader argument in the README, but it seems like it is only available in the clojure.tools.reader.reader-types namespace. Is that intentional? Perhaps making it available in the clojure.tools.reader and/or clojure.tools.reader.edn namespace might be more convenient for the user of the library? I understand that the current implementation of read-line is Java-specific, so maybe this is the only reasonable way to expose it. In any case, documenting the namespace in which this enhanced read-line is available in the README would be good. |
| Comments |
| Comment by Nicola Mometto [ 15/Mar/13 10:26 AM ] |
|
I added more documentation on the readme, making it clear in which namespace read-line is defined. read-line definitely belongs in the c.t.r.reader-types namespace, since it works on reader-types and returns a string, so it's not a reader function. I hope this addresses this ticket, I'm closing it, feel free to reopen it you think the doc needs to be more clear. |
| Comment by Andy Fingerhut [ 15/Mar/13 7:04 PM ] |
|
Looks thoroughly documented to me – above and beyond what I would have asked for. Thanks. |
[TRDR-4] Typo: Change 'end' to 'edn' in 'clojure.tools.reader.end/read-string' in README Created: 15/Mar/13 Updated: 15/Mar/13 Resolved: 15/Mar/13 |
|
| Status: | Closed |
| Project: | tools.reader |
| Component/s: | None |
| Affects Version/s: | None |
| Fix Version/s: | None |
| Type: | Enhancement | Priority: | Trivial |
| Reporter: | Andy Fingerhut | Assignee: | Nicola Mometto |
| Resolution: | Completed | Votes: | 0 |
| Labels: | None | ||
| Description |
|
Just a miniscule typo |
| Comments |
| Comment by Nicola Mometto [ 15/Mar/13 9:50 AM ] |
|
Fixed, thanks |
[TRDR-2] Fix a few README typos Created: 14/Feb/13 Updated: 15/Feb/13 Resolved: 15/Feb/13 |
|
| Status: | Resolved |
| Project: | tools.reader |
| Component/s: | None |
| Affects Version/s: | None |
| Fix Version/s: | None |
| Type: | Enhancement | Priority: | Minor |
| Reporter: | Andy Fingerhut | Assignee: | Nicola Mometto |
| Resolution: | Completed | Votes: | 0 |
| Labels: | None | ||
| Attachments: |
trdr-2-fix-readme-typos-patch-v1.txt
|
| Patch: | Code |
| Description |
|
There are a few typos in the README |
| Comments |
| Comment by Andy Fingerhut [ 14/Feb/13 3:16 PM ] |
|
Nothing major here. Just a few typo fixes for the README that I found. |
| Comment by Nicola Mometto [ 15/Feb/13 4:19 AM ] |
|
Thanks, applied |
[TRDR-1] read-char returns nil for some input types, on first attempt to read a char Created: 12/Feb/13 Updated: 12/Feb/13 Resolved: 12/Feb/13 |
|
| Status: | Resolved |
| Project: | tools.reader |
| Component/s: | None |
| Affects Version/s: | None |
| Fix Version/s: | None |
| Type: | Defect | Priority: | Major |
| Reporter: | Andy Fingerhut | Assignee: | Andy Fingerhut |
| Resolution: | Completed | Votes: | 0 |
| Labels: | None | ||
| Attachments: |
trdr-1-correct-read-char-v1.txt
|
| Patch: | Code |
| Description |
|
The return value from Java's .read method is -1 for EOF. A couple of condition checks in the code appear to be reversed. See the patch. |
| Comments |
| Comment by Nicola Mometto [ 12/Feb/13 4:47 PM ] |
|
Thanks, tests for reader-types are now on the TODO list |
)