<< Back to previous view

[CLJCLR-37] spit does not truncate, leaves tail of original file if output is shorter length. Created: 29/Sep/14  Updated: 28/Nov/14  Resolved: 28/Nov/14

Status: Resolved
Project: ClojureCLR
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Major
Reporter: David Miller Assignee: David Miller
Resolution: Declined Votes: 0
Labels: None


 Description   

For example:

(spit "c:/tmp/spit.txt" "012345")
(spit "c:/tmp/spit.txt" "BB")

Results in a file containing: BB345.

Reported via email by cees van Kemenade.



 Comments   
Comment by David Miller [ 28/Nov/14 10:27 AM ]

spit with a string argument defaults to opening a FileStream (wrapped in a StreamWriter) in mode FileMode.CreateOrNew. This will work whether or not the file exists. To default to Truncate will fail if the file does not exist. For this reason, I don't want to make it the default. I'm also not sure how to play with tests of file existence versus given file-mode to determine what action to take.

If you know the file exists and want to truncate, call spit with that file mode.

(spit "filename" "test" :file-mode System.IO.FileMode/Truncate)

Comment by David Miller [ 28/Nov/14 10:28 AM ]

Correct action is to call spit specifying the file mode:

(spit "c:/tmp/spit.txt" "BB" :file-mode System.IO.FileMode/Truncate)





[CLJCLR-49] ArraySeq.createFromObject broken on arrays of value types Created: 14/Nov/14  Updated: 28/Nov/14  Resolved: 28/Nov/14

Status: Resolved
Project: ClojureCLR
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Major
Reporter: Ramsey Nasser Assignee: David Miller
Resolution: Completed Votes: 0
Labels: None
Environment:

Arcadia


Attachments: File CLJCLR-49.diff    

 Description   

Calling seq on CLR arrays eventually comes to ArraySeq.createFromObject, which handles arrays of primitive and reference types correctly, but throws an exception when called with an array of value types. Value types have a TypeCode of Object, just like reference types, so they both fall into the branch on line 91 of ArraySeq, which attempts an object[] prefix cast. Casting an array of value types to an array of reference types is not allowed, so this will throw an exception.

Original Arcadia issue: https://github.com/arcadia-unity/Arcadia/issues/75. The example case makes use of Unity types, but the same should hold for any CLR environment.



 Comments   
Comment by Ramsey Nasser [ 14/Nov/14 1:19 PM ]

The provided patch fixes the problem by leveraging CLR generics and the already present TypedArraySeq class for value types. The following changes were needed

1. TypedArraySeq was made public and non-abstract, and given generic implementations for its previously abstract methods
2. The default case in ArraySeq.createFromObject was replaced by a runtime reflective instantiation of the correct generic version of TypedArraySeq for the input type

Comment by Ramsey Nasser [ 14/Nov/14 2:41 PM ]

The only odd part of the patch is the ConvertNum implementation, which right now just casts to the generic type, mirroring what ArraySeq_object does. Not sure what else to do there, but I have other questions about ConvertNum.

TypedArraySeq could conceivably replace all the hard coded ArraySeq_* subclasses. This would result in much smaller (~65% smaller), simpler code. They seem to all do the exact same thing, except in the ConvertNum method, which brings up a few questions. Why is ConvertNum there, and why is it so concerned with numbers? It is only used in the IList.IndexOf implementation, where input is run through a Util.IsNumeric check before converting to a number via ConvertNum, and only then looking up the index. This means that IndexOf will always return -1 for anything that is not numeric. Is there a reason for this behavior, or is it a bug? As usual I'm not aware of the farther reaching implications of these implementation details.

If it is a bug, then I propose we replace ConvertNum with ConvertToType or just Convert that converts a value to the generic type of the TypedArraySeq and remove the Util.IsNumeric check from IndexOf. I can provide this patch.

Comment by David Miller [ 28/Nov/14 9:09 AM ]

Proposed patch is fine.

The hard-coded ArraySeq_* classes were provided to parallel the JVM implementation. My usual obsession with not deviating more than necessary. Do we really need them? No.

The ConvertNum became a bug. Originally, there was no TypedArraySeq. I then introduced TypedArraySeq to handle the numeric specializations only – there was also an UntypedArraySeq for non-numeric arrays. TypedArraySeq being only for numerics, the ConvertNum and the test for Util.IsNumeric make sense. The latter was to make sure that the call to Util.ConvertToWhatever would not blow up. (Need a TryConvertToWhatever, I supposed.) When I got rid of UntypedArraySeq in favor of TypedArraySeq<Object>, the bug was introduced.

In the fix, I've changed ConvertNum to Convert. To get the Util.IsNumeric call in for numeric types, I added a NumericArraySeq specialization of TypedArraySeq. I have left in the ArraySeq_* classes, just in case. This could be revisited. (As could every other line of code.

Comment by David Miller [ 28/Nov/14 9:13 AM ]

Patch applied + related work.
Commit b27ff78, 2014.11.28.





Generated at Fri Nov 28 20:35:16 CST 2014 using JIRA 4.4#649-r158309.