<< Back to previous view

[CLJ-1074] Read/print round-trip for +/-Infinity and NaN Created: 21/Sep/12  Updated: 07/Sep/17  Resolved: 07/Sep/17

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: Release 1.4
Fix Version/s: Release 1.9

Type: Defect Priority: Major
Reporter: Colin Jones Assignee: Unassigned
Resolution: Completed Votes: 7
Labels: print, reader

Attachments: Text File 0001-CLJ-1074-add-Inf-Inf-Inf-NaN.patch     Text File 0001-CLJ-1074-add-Inf-Inf-Inf-NaN-v2.patch     Text File 0001-Read-Infinity-and-NaN.patch     Text File clj-1074-2.patch     Text File clj-1074-3.patch     Text File clj-1074-5.patch     Text File clj-1074-6.patch     Text File clj-1074-7.patch     Text File clj-1074-8.patch     Text File clj-1074-read-infinity-and-nan-patch-v2-plus-edn-reader.patch    
Patch: Code and Test
Approval: Ok

 Description   

A few float-related forms (namely, Double/POSITIVE_INFINITY, Double/NEGATIVE_INFINITY, Double/NaN) are not eval-able after a round-trip via

(read-string (binding [*print-dup* true] (pr-str f))

The two options I see are to provide print-method implementations for these and their Float cousins, or to make Infinity, -Infinity, +Infinity, and NaN readable values. Since it sounds like edn may want to provide a spec for these values (see https://groups.google.com/d/topic/clojure-dev/LeJpOhHxESs/discussion and https://github.com/edn-format/edn/issues/2), I think making these values directly readable as already printed is preferable. Something like Double/POSITIVE_INFINITY seems too low-level from edn's perspective, as it would refer to a Java class and constant.

I'm attaching a patch implementing reader support for ##Inf, ##-Inf, ##+Inf, and ##NaN.

Patch: clj-1074-8.patch



 Comments   
Comment by Timothy Baldridge [ 03/Dec/12 11:34 AM ]

Please bring this up on clojure-dev. We'll be able to vet this ticket after that.

Comment by Colin Jones [ 03/Dec/12 1:18 PM ]

Should I respond to my original clojure-dev post about this (linked in the issue description above), or start a new one?

Comment by Andy Fingerhut [ 24/May/13 1:11 PM ]

Patch clj-1074-read-infinity-and-nan-patch-v2.txt dated May 24 2013 is identical to 0001-Read-Infinity-and-NaN.patch dated Sep 21 2012, except it applies cleanly to latest master. The older patch conflicts with a recent commit made for CLJ-873.

Comment by Nicola Mometto [ 25/May/13 11:55 AM ]

clj-1074-read-infinity-and-nan-patch-v2-plus-edn-reader.patch is the same as clj-1074-read-infinity-and-nan-patch-v2.txt except it patches EdnReader too, but it must be applied after #CLJ-873 0001-Fix-CLJ-873-for-EdnReader-too.patch get merged

Comment by Andrew Tarzwell [ 12/Feb/15 12:01 PM ]

We're running into this bug now, applying the patch clj-1074-read-infinity-and-nan-patch-v2-plus-edn-reader.patch seems to resolve it on 1.7 master, but it would be an improvement to not depend on a patched version.

Is there a fix in the works? Or a more up to date conversation on why this hasn't been addressed?

Thanks,
Andrew

Comment by Andy Fingerhut [ 12/Feb/15 12:23 PM ]

Andrew, the tools.reader library provides this enhancement today, if you would prefer using unpatched software, and if it meets your needs. There are a few other small differences between it and the reader built into Clojure, which you can see near the end of its README: https://github.com/clojure/tools.reader

As far as why it hasn't been addressed yet, I think a short accurate answer is that the Clojure developers have been working on other issues, and this one has not been high enough priority to be addressed yet (disclaimer: This is in no way an official answer, just the best guess of an observer who watches this stuff too much).

I see you have voted on the ticket. Good. More votes can in some cases influence the Clojure developers to respond to a ticket earlier rather than later. You may try to persuade others to vote on it, too, if you wish.

If there is some production use of Clojure hindered by the lack of a fix, bringing this up in the Clojure or Clojure Dev Google groups couldn't hurt (signed CA on file required for Clojure Dev group membership – see http://clojure.org/contributing )

Comment by Andrew Tarzwell [ 12/Feb/15 1:46 PM ]

Andy,

Thank you for the quick response, I was unaware of tools.reader having this fixed. That should do us for now.

Comment by Alex Miller [ 24/Oct/16 9:43 AM ]

Refreshed patch to apply to current master. No semantic changes, attribution retained.

Comment by Alex Miller [ 30/Jun/17 9:03 AM ]

Updated patch so it would apply cleanly, no other changes, attribution retained.

Comment by Rich Hickey [ 06/Sep/17 8:57 AM ]

polluting the top-level is not an option. I propose a new dispatch macro character #, and these values: ##Inf ##-Inf ##NaN. We'll need patches for LispReader, edn reader, and the edn spec

Comment by Nicola Mometto [ 06/Sep/17 9:01 AM ]

Should we also change how +/-Inf and NaN print then?

Comment by Alex Miller [ 06/Sep/17 9:13 AM ]

Yes.

Comment by Nicola Mometto [ 06/Sep/17 9:42 AM ]

Attached patch including support for ##Inf ##+Inf ##-Inf and ##NaN (I've included ##+Inf for consistency, but let me know if it's not desiderable.) I'll make a PR on the edn spec about it once I know whether or not to include ##+Inf

Comment by Stuart Halloway [ 06/Sep/17 1:41 PM ]

Thanks Nicola!

Comment by Stuart Halloway [ 06/Sep/17 1:47 PM ]

Rich would like to have

  • remove the +Inf
  • rename SpecialSymbolReader to SymbolicValueReader
Comment by Rich Hickey [ 06/Sep/17 1:48 PM ]

Yes, thanks Nicola. Let's go with SymbolicValueReader and not do +Inf

Comment by Nicola Mometto [ 06/Sep/17 1:55 PM ]

Updated patch as per comments

Comment by Alex Miller [ 07/Sep/17 8:20 AM ]

New patch fixes printing for ##NaN, modifies the exception handling in the readers slightly, and adds some more printing tests.

Comment by Alex Miller [ 07/Sep/17 9:03 AM ]

Did a perf check on number printing with this:

(use 'criterium.core)				
(def v (vec (range 1000)))		 
(def vd (vec (range 0.0 1000.0 1.0)))
(bench (pr-str v))
(bench (pr-str vd))

Results:

int vector:     168.3 µs -> 231.1 µs (37% slower)
double vector:  185.2 µs -> 270.7 µs (46% slower
Comment by Alex Miller [ 07/Sep/17 11:46 AM ]

Dropping a new patch that doesn't put a conditional in the Number case and instead adds a print-method for Double. Updated #s:

long vector:    168.3 µs -> 166.6 µs (wash)
double vector:  185.2 µs -> 207.6 µs (12% slower)

That seems tolerable.

Comment by Alex Miller [ 07/Sep/17 1:31 PM ]

-7 patch adds print support and tests for float infinity and nan.

Comment by Alex Miller [ 07/Sep/17 1:44 PM ]

-8 patch makes the double symbol reader tests more precise by checking value, not just type

Generated at Thu Jan 18 12:26:19 CST 2018 using JIRA 4.4#649-r158309.