You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@commons.apache.org by Gilles <gi...@harfang.homelinux.org> on 2018/02/13 09:31:33 UTC

Re: commons-csv git commit: CSV-216: Avoid Arrays.copyOf()

On Tue, 13 Feb 2018 00:16:13 +0000 (UTC), stain@apache.org wrote:
> Repository: commons-csv
> Updated Branches:
>   refs/heads/CSV-216 637ad2d7a -> f66a83901
>
>
> CSV-216: Avoid Arrays.copyOf()

Why?

> as .clone() will do

We should rather avoid using "clone()".

Gilles

> -- at least until someone tries to do
> .withValue(x) in an out-of-range column
>
>
> Project: http://git-wip-us.apache.org/repos/asf/commons-csv/repo
> Commit: 
> http://git-wip-us.apache.org/repos/asf/commons-csv/commit/f66a8390
> Tree: 
> http://git-wip-us.apache.org/repos/asf/commons-csv/tree/f66a8390
> Diff: 
> http://git-wip-us.apache.org/repos/asf/commons-csv/diff/f66a8390
>
> Branch: refs/heads/CSV-216
> Commit: f66a83901bd026369a2e8d522bd567eb2ef3f8c0
> Parents: 637ad2d
> Author: Stian Soiland-Reyes <st...@apache.org>
> Authored: Fri Feb 9 16:49:51 2018 +0000
> Committer: Stian Soiland-Reyes <st...@apache.org>
> Committed: Tue Feb 13 00:14:52 2018 +0000
>
> 
> ----------------------------------------------------------------------
>  src/main/java/org/apache/commons/csv/CSVRecord.java | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> ----------------------------------------------------------------------
>
>
> 
> http://git-wip-us.apache.org/repos/asf/commons-csv/blob/f66a8390/src/main/java/org/apache/commons/csv/CSVRecord.java
> 
> ----------------------------------------------------------------------
> diff --git a/src/main/java/org/apache/commons/csv/CSVRecord.java
> b/src/main/java/org/apache/commons/csv/CSVRecord.java
> index 979119f..2be5c49 100644
> --- a/src/main/java/org/apache/commons/csv/CSVRecord.java
> +++ b/src/main/java/org/apache/commons/csv/CSVRecord.java
> @@ -199,7 +199,7 @@ public class CSVRecord implements Serializable,
> Iterable<String> {
>      public final CSVRecord immutable() {
>      	if (isMutable()) {
>  	    	// Subclass is probably CSVMutableRecord, freeze values
> -	    	String[] frozenValue = Arrays.copyOf(values, values.length);
> +		String[] frozenValue = values.clone();
>  	    	return new CSVRecord(frozenValue, mapping, comment,
> recordNumber, characterPosition);
>      	} else {
>      		return this;
> @@ -260,7 +260,7 @@ public class CSVRecord implements Serializable,
> Iterable<String> {
>      	if (isMutable()) {
>      		return this;
>      	}
> -		String[] newValues = Arrays.copyOf(values, values.length);
> +		String[] newValues = values.clone();
>      	return new CSVMutableRecord(newValues, mapping, comment,
> recordNumber, characterPosition);
>  	}


---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org


Re: commons-csv git commit: CSV-216: Avoid Arrays.copyOf()

Posted by Stian Soiland-Reyes <st...@apache.org>.
On Fri, 16 Feb 2018 20:11:11 +0100, Gilles <gi...@harfang.homelinux.org> wrote:
> Bottom-line: Don't use clone except to copy arrays.
> So I'd rephrase my comment on the commit: let "clone()" in place
> but with the comment that it's the only acceptable use of it.

OK, I've added such comments :)

(It was also used for copying the headers, which was also String[])

-- 
Stian Soiland-Reyes
The University of Manchester
http://www.esciencelab.org.uk/
http://orcid.org/0000-0001-9842-9718


---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org


Re: commons-csv git commit: CSV-216: Avoid Arrays.copyOf()

Posted by Gilles <gi...@harfang.homelinux.org>.
On Fri, 16 Feb 2018 08:33:46 -0800, Chas Honton wrote:
> Read Bloch.  Don’t optimize until you have proven that this bit of
> code is causing a significant performance hit.
> arrayCopy can and is inlined by jit

https://www.artima.com/intv/bloch13.html

Bottom-line: Don't use clone except to copy arrays.
So I'd rephrase my comment on the commit: let "clone()" in place
but with the comment that it's the only acceptable use of it.

Regards,
Gilles

>
> Chas
>
>> On Feb 16, 2018, at 5:53 AM, Stian Soiland-Reyes <st...@apache.org> 
>> wrote:
>>
>> String is still a type of Object (requiring GC handling of reference
>> counters), as you can do Object[] o = stringArray.clone(); without
>> casting.  (other way not so)
>>
>> https://www.javaspecialists.eu/archive/Issue124.html says there is
>> hardly any difference, so I don't think we need to fight this one 
>> out
>> and can just go with the cleanest code :)
>>
>>> On 16 February 2018 at 11:22, sebb <se...@gmail.com> wrote:
>>>> On 16 February 2018 at 10:01, Stian Soiland-Reyes 
>>>> <st...@apache.org> wrote:
>>>> I agree in general for .clone() on objects - and I'm trying to 
>>>> move
>>>> away from using .clone() in that Commons RDF fluent 
>>>> mutable/immutable
>>>> builder (See COMMONSRDF-49).
>>>>
>>>> But this is an Object[] - a native type.
>>>
>>> Huh?
>>>
>>> The code says String[], which is native as it's final.
>>>
>>> Object[] may not be native
>>>
>>>> I must admit I am not sure what is really the preferred way - I
>>>> thought .clone() is best as the VM can copy an array natively 
>>>> however
>>>> it wants, while with Arrays.copyOf it might have to check if 
>>>> length is
>>>> the same before doing anything clever.
>>>
>>> In the specific case of String[] it might be OK to use clone, but 
>>> in
>>> general it's better to use the proper JVM methods and not try to
>>> second-guess how efficient they are.
>>>
>>>>> On 13 February 2018 at 18:58, Gilles 
>>>>> <gi...@harfang.homelinux.org> wrote:
>>>>>> On Tue, 13 Feb 2018 18:40:13 +0000, sebb wrote:
>>>>>>
>>>>>>> On 13 February 2018 at 09:31, Gilles 
>>>>>>> <gi...@harfang.homelinux.org> wrote:
>>>>>>>
>>>>>>>> On Tue, 13 Feb 2018 00:16:13 +0000 (UTC), stain@apache.org 
>>>>>>>> wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>> Repository: commons-csv
>>>>>>>> Updated Branches:
>>>>>>>>  refs/heads/CSV-216 637ad2d7a -> f66a83901
>>>>>>>>
>>>>>>>>
>>>>>>>> CSV-216: Avoid Arrays.copyOf()
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> Why?
>>>>>>
>>>>>>
>>>>>> Agreed
>>>>>>
>>>>>>>> as .clone() will do
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> We should rather avoid using "clone()".
>>>>>>
>>>>>>
>>>>>> Again: why?
>>>>>
>>>>>
>>>>> There are many discussions about this topic on the web.[1]
>>>>> My point is that using "clone()" is not good advice.
>>>>>
>>>>> Gilles
>>>>>
>>>>> [1] E.g. http://vojtechruzicka.com/java-cloning-problems/
>>>>>
>>>>>
>>>>>> It's not clear why Arrays.copyOf(0 is considered bad, nor why 
>>>>>> clone()
>>>>>> is considered bad.
>>>>>>
>>>>>>
>>>>>>> Gilles
>>>>>>>
>>>>>>>
>>>>>>>> -- at least until someone tries to do
>>>>>>>> .withValue(x) in an out-of-range column
>>>>>>>>
>>>>>>>>
>>>>>>>> Project: 
>>>>>>>> http://git-wip-us.apache.org/repos/asf/commons-csv/repo
>>>>>>>> Commit:
>>>>>>>> 
>>>>>>>> http://git-wip-us.apache.org/repos/asf/commons-csv/commit/f66a8390
>>>>>>>> Tree: 
>>>>>>>> http://git-wip-us.apache.org/repos/asf/commons-csv/tree/f66a8390
>>>>>>>> Diff: 
>>>>>>>> http://git-wip-us.apache.org/repos/asf/commons-csv/diff/f66a8390
>>>>>>>>
>>>>>>>> Branch: refs/heads/CSV-216
>>>>>>>> Commit: f66a83901bd026369a2e8d522bd567eb2ef3f8c0
>>>>>>>> Parents: 637ad2d
>>>>>>>> Author: Stian Soiland-Reyes <st...@apache.org>
>>>>>>>> Authored: Fri Feb 9 16:49:51 2018 +0000
>>>>>>>> Committer: Stian Soiland-Reyes <st...@apache.org>
>>>>>>>> Committed: Tue Feb 13 00:14:52 2018 +0000
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> 
>>>>>>>> ----------------------------------------------------------------------
>>>>>>>> src/main/java/org/apache/commons/csv/CSVRecord.java | 4 ++--
>>>>>>>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>>>>>>>
>>>>>>>>
>>>>>>>> 
>>>>>>>> ----------------------------------------------------------------------
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> 
>>>>>>>> http://git-wip-us.apache.org/repos/asf/commons-csv/blob/f66a8390/src/main/java/org/apache/commons/csv/CSVRecord.java
>>>>>>>>
>>>>>>>>
>>>>>>>> 
>>>>>>>> ----------------------------------------------------------------------
>>>>>>>> diff --git 
>>>>>>>> a/src/main/java/org/apache/commons/csv/CSVRecord.java
>>>>>>>> b/src/main/java/org/apache/commons/csv/CSVRecord.java
>>>>>>>> index 979119f..2be5c49 100644
>>>>>>>> --- a/src/main/java/org/apache/commons/csv/CSVRecord.java
>>>>>>>> +++ b/src/main/java/org/apache/commons/csv/CSVRecord.java
>>>>>>>> @@ -199,7 +199,7 @@ public class CSVRecord implements 
>>>>>>>> Serializable,
>>>>>>>> Iterable<String> {
>>>>>>>>     public final CSVRecord immutable() {
>>>>>>>>        if (isMutable()) {
>>>>>>>>                // Subclass is probably CSVMutableRecord, 
>>>>>>>> freeze values
>>>>>>>> -               String[] frozenValue = Arrays.copyOf(values,
>>>>>>>> values.length);
>>>>>>>> +               String[] frozenValue = values.clone();
>>>>>>>>                return new CSVRecord(frozenValue, mapping, 
>>>>>>>> comment,
>>>>>>>> recordNumber, characterPosition);
>>>>>>>>        } else {
>>>>>>>>                return this;
>>>>>>>> @@ -260,7 +260,7 @@ public class CSVRecord implements 
>>>>>>>> Serializable,
>>>>>>>> Iterable<String> {
>>>>>>>>        if (isMutable()) {
>>>>>>>>                return this;
>>>>>>>>        }
>>>>>>>> -               String[] newValues = Arrays.copyOf(values,
>>>>>>>> values.length);
>>>>>>>> +               String[] newValues = values.clone();
>>>>>>>>        return new CSVMutableRecord(newValues, mapping, 
>>>>>>>> comment,
>>>>>>>> recordNumber, characterPosition);
>>>>>>>>        }


---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org


Re: commons-csv git commit: CSV-216: Avoid Arrays.copyOf()

Posted by Chas Honton <ch...@honton.org>.
Read Bloch.  Don’t optimize until you have proven that this bit of code is causing a significant performance hit. 
arrayCopy can and is inlined by jit. 

Chas

> On Feb 16, 2018, at 5:53 AM, Stian Soiland-Reyes <st...@apache.org> wrote:
> 
> String is still a type of Object (requiring GC handling of reference
> counters), as you can do Object[] o = stringArray.clone(); without
> casting.  (other way not so)
> 
> https://www.javaspecialists.eu/archive/Issue124.html says there is
> hardly any difference, so I don't think we need to fight this one out
> and can just go with the cleanest code :)
> 
>> On 16 February 2018 at 11:22, sebb <se...@gmail.com> wrote:
>>> On 16 February 2018 at 10:01, Stian Soiland-Reyes <st...@apache.org> wrote:
>>> I agree in general for .clone() on objects - and I'm trying to move
>>> away from using .clone() in that Commons RDF fluent mutable/immutable
>>> builder (See COMMONSRDF-49).
>>> 
>>> But this is an Object[] - a native type.
>> 
>> Huh?
>> 
>> The code says String[], which is native as it's final.
>> 
>> Object[] may not be native
>> 
>>> I must admit I am not sure what is really the preferred way - I
>>> thought .clone() is best as the VM can copy an array natively however
>>> it wants, while with Arrays.copyOf it might have to check if length is
>>> the same before doing anything clever.
>> 
>> In the specific case of String[] it might be OK to use clone, but in
>> general it's better to use the proper JVM methods and not try to
>> second-guess how efficient they are.
>> 
>>>> On 13 February 2018 at 18:58, Gilles <gi...@harfang.homelinux.org> wrote:
>>>>> On Tue, 13 Feb 2018 18:40:13 +0000, sebb wrote:
>>>>> 
>>>>>> On 13 February 2018 at 09:31, Gilles <gi...@harfang.homelinux.org> wrote:
>>>>>> 
>>>>>>> On Tue, 13 Feb 2018 00:16:13 +0000 (UTC), stain@apache.org wrote:
>>>>>>> 
>>>>>>> 
>>>>>>> Repository: commons-csv
>>>>>>> Updated Branches:
>>>>>>>  refs/heads/CSV-216 637ad2d7a -> f66a83901
>>>>>>> 
>>>>>>> 
>>>>>>> CSV-216: Avoid Arrays.copyOf()
>>>>>> 
>>>>>> 
>>>>>> 
>>>>>> Why?
>>>>> 
>>>>> 
>>>>> Agreed
>>>>> 
>>>>>>> as .clone() will do
>>>>>> 
>>>>>> 
>>>>>> 
>>>>>> We should rather avoid using "clone()".
>>>>> 
>>>>> 
>>>>> Again: why?
>>>> 
>>>> 
>>>> There are many discussions about this topic on the web.[1]
>>>> My point is that using "clone()" is not good advice.
>>>> 
>>>> Gilles
>>>> 
>>>> [1] E.g. http://vojtechruzicka.com/java-cloning-problems/
>>>> 
>>>> 
>>>>> It's not clear why Arrays.copyOf(0 is considered bad, nor why clone()
>>>>> is considered bad.
>>>>> 
>>>>> 
>>>>>> Gilles
>>>>>> 
>>>>>> 
>>>>>>> -- at least until someone tries to do
>>>>>>> .withValue(x) in an out-of-range column
>>>>>>> 
>>>>>>> 
>>>>>>> Project: http://git-wip-us.apache.org/repos/asf/commons-csv/repo
>>>>>>> Commit:
>>>>>>> http://git-wip-us.apache.org/repos/asf/commons-csv/commit/f66a8390
>>>>>>> Tree: http://git-wip-us.apache.org/repos/asf/commons-csv/tree/f66a8390
>>>>>>> Diff: http://git-wip-us.apache.org/repos/asf/commons-csv/diff/f66a8390
>>>>>>> 
>>>>>>> Branch: refs/heads/CSV-216
>>>>>>> Commit: f66a83901bd026369a2e8d522bd567eb2ef3f8c0
>>>>>>> Parents: 637ad2d
>>>>>>> Author: Stian Soiland-Reyes <st...@apache.org>
>>>>>>> Authored: Fri Feb 9 16:49:51 2018 +0000
>>>>>>> Committer: Stian Soiland-Reyes <st...@apache.org>
>>>>>>> Committed: Tue Feb 13 00:14:52 2018 +0000
>>>>>>> 
>>>>>>> 
>>>>>>> 
>>>>>>> ----------------------------------------------------------------------
>>>>>>> src/main/java/org/apache/commons/csv/CSVRecord.java | 4 ++--
>>>>>>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>>>>>> 
>>>>>>> 
>>>>>>> ----------------------------------------------------------------------
>>>>>>> 
>>>>>>> 
>>>>>>> 
>>>>>>> 
>>>>>>> 
>>>>>>> 
>>>>>>> http://git-wip-us.apache.org/repos/asf/commons-csv/blob/f66a8390/src/main/java/org/apache/commons/csv/CSVRecord.java
>>>>>>> 
>>>>>>> 
>>>>>>> ----------------------------------------------------------------------
>>>>>>> diff --git a/src/main/java/org/apache/commons/csv/CSVRecord.java
>>>>>>> b/src/main/java/org/apache/commons/csv/CSVRecord.java
>>>>>>> index 979119f..2be5c49 100644
>>>>>>> --- a/src/main/java/org/apache/commons/csv/CSVRecord.java
>>>>>>> +++ b/src/main/java/org/apache/commons/csv/CSVRecord.java
>>>>>>> @@ -199,7 +199,7 @@ public class CSVRecord implements Serializable,
>>>>>>> Iterable<String> {
>>>>>>>     public final CSVRecord immutable() {
>>>>>>>        if (isMutable()) {
>>>>>>>                // Subclass is probably CSVMutableRecord, freeze values
>>>>>>> -               String[] frozenValue = Arrays.copyOf(values,
>>>>>>> values.length);
>>>>>>> +               String[] frozenValue = values.clone();
>>>>>>>                return new CSVRecord(frozenValue, mapping, comment,
>>>>>>> recordNumber, characterPosition);
>>>>>>>        } else {
>>>>>>>                return this;
>>>>>>> @@ -260,7 +260,7 @@ public class CSVRecord implements Serializable,
>>>>>>> Iterable<String> {
>>>>>>>        if (isMutable()) {
>>>>>>>                return this;
>>>>>>>        }
>>>>>>> -               String[] newValues = Arrays.copyOf(values,
>>>>>>> values.length);
>>>>>>> +               String[] newValues = values.clone();
>>>>>>>        return new CSVMutableRecord(newValues, mapping, comment,
>>>>>>> recordNumber, characterPosition);
>>>>>>>        }
>>>> 
>>>> 
>>>> 
>>>> ---------------------------------------------------------------------
>>>> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
>>>> For additional commands, e-mail: dev-help@commons.apache.org
>>>> 
>>> 
>>> 
>>> 
>>> --
>>> Stian Soiland-Reyes
>>> http://orcid.org/0000-0001-9842-9718
>>> 
>>> ---------------------------------------------------------------------
>>> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
>>> For additional commands, e-mail: dev-help@commons.apache.org
>>> 
>> 
>> ---------------------------------------------------------------------
>> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
>> For additional commands, e-mail: dev-help@commons.apache.org
>> 
> 
> 
> 
> -- 
> Stian Soiland-Reyes
> http://orcid.org/0000-0001-9842-9718
> 
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
> For additional commands, e-mail: dev-help@commons.apache.org
> 
> 

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org


Re: commons-csv git commit: CSV-216: Avoid Arrays.copyOf()

Posted by Gilles <gi...@harfang.homelinux.org>.
On Fri, 16 Feb 2018 13:53:28 +0000, Stian Soiland-Reyes wrote:
> String is still a type of Object (requiring GC handling of reference
> counters), as you can do Object[] o = stringArray.clone(); without
> casting.  (other way not so)
>
> https://www.javaspecialists.eu/archive/Issue124.html says there is
> hardly any difference, so I don't think we need to fight this one out
> and can just go with the cleanest code :)

+1

That was my point.
Before "Arrays" (Java 1.6), I also used to "clone" arrays, in
preference to "System.arraycopy" ;-)

Gilles

>
> On 16 February 2018 at 11:22, sebb <se...@gmail.com> wrote:
>> On 16 February 2018 at 10:01, Stian Soiland-Reyes <st...@apache.org> 
>> wrote:
>>> I agree in general for .clone() on objects - and I'm trying to move
>>> away from using .clone() in that Commons RDF fluent 
>>> mutable/immutable
>>> builder (See COMMONSRDF-49).
>>>
>>> But this is an Object[] - a native type.
>>
>> Huh?
>>
>> The code says String[], which is native as it's final.
>>
>> Object[] may not be native
>>
>>> I must admit I am not sure what is really the preferred way - I
>>> thought .clone() is best as the VM can copy an array natively 
>>> however
>>> it wants, while with Arrays.copyOf it might have to check if length 
>>> is
>>> the same before doing anything clever.
>>
>> In the specific case of String[] it might be OK to use clone, but in
>> general it's better to use the proper JVM methods and not try to
>> second-guess how efficient they are.
>>
>>> On 13 February 2018 at 18:58, Gilles <gi...@harfang.homelinux.org> 
>>> wrote:
>>>> On Tue, 13 Feb 2018 18:40:13 +0000, sebb wrote:
>>>>>
>>>>> On 13 February 2018 at 09:31, Gilles 
>>>>> <gi...@harfang.homelinux.org> wrote:
>>>>>>
>>>>>> On Tue, 13 Feb 2018 00:16:13 +0000 (UTC), stain@apache.org 
>>>>>> wrote:
>>>>>>>
>>>>>>>
>>>>>>> Repository: commons-csv
>>>>>>> Updated Branches:
>>>>>>>   refs/heads/CSV-216 637ad2d7a -> f66a83901
>>>>>>>
>>>>>>>
>>>>>>> CSV-216: Avoid Arrays.copyOf()
>>>>>>
>>>>>>
>>>>>>
>>>>>> Why?
>>>>>
>>>>>
>>>>> Agreed
>>>>>
>>>>>>> as .clone() will do
>>>>>>
>>>>>>
>>>>>>
>>>>>> We should rather avoid using "clone()".
>>>>>
>>>>>
>>>>> Again: why?
>>>>
>>>>
>>>> There are many discussions about this topic on the web.[1]
>>>> My point is that using "clone()" is not good advice.
>>>>
>>>> Gilles
>>>>
>>>> [1] E.g. http://vojtechruzicka.com/java-cloning-problems/
>>>>
>>>>
>>>>> It's not clear why Arrays.copyOf(0 is considered bad, nor why 
>>>>> clone()
>>>>> is considered bad.
>>>>>
>>>>>
>>>>>> Gilles
>>>>>>
>>>>>>
>>>>>>> -- at least until someone tries to do
>>>>>>> .withValue(x) in an out-of-range column
>>>>>>>
>>>>>>>
>>>>>>> Project: 
>>>>>>> http://git-wip-us.apache.org/repos/asf/commons-csv/repo
>>>>>>> Commit:
>>>>>>> 
>>>>>>> http://git-wip-us.apache.org/repos/asf/commons-csv/commit/f66a8390
>>>>>>> Tree: 
>>>>>>> http://git-wip-us.apache.org/repos/asf/commons-csv/tree/f66a8390
>>>>>>> Diff: 
>>>>>>> http://git-wip-us.apache.org/repos/asf/commons-csv/diff/f66a8390
>>>>>>>
>>>>>>> Branch: refs/heads/CSV-216
>>>>>>> Commit: f66a83901bd026369a2e8d522bd567eb2ef3f8c0
>>>>>>> Parents: 637ad2d
>>>>>>> Author: Stian Soiland-Reyes <st...@apache.org>
>>>>>>> Authored: Fri Feb 9 16:49:51 2018 +0000
>>>>>>> Committer: Stian Soiland-Reyes <st...@apache.org>
>>>>>>> Committed: Tue Feb 13 00:14:52 2018 +0000
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> 
>>>>>>> ----------------------------------------------------------------------
>>>>>>>  src/main/java/org/apache/commons/csv/CSVRecord.java | 4 ++--
>>>>>>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>>>>>>
>>>>>>>
>>>>>>> 
>>>>>>> ----------------------------------------------------------------------
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> 
>>>>>>> http://git-wip-us.apache.org/repos/asf/commons-csv/blob/f66a8390/src/main/java/org/apache/commons/csv/CSVRecord.java
>>>>>>>
>>>>>>>
>>>>>>> 
>>>>>>> ----------------------------------------------------------------------
>>>>>>> diff --git 
>>>>>>> a/src/main/java/org/apache/commons/csv/CSVRecord.java
>>>>>>> b/src/main/java/org/apache/commons/csv/CSVRecord.java
>>>>>>> index 979119f..2be5c49 100644
>>>>>>> --- a/src/main/java/org/apache/commons/csv/CSVRecord.java
>>>>>>> +++ b/src/main/java/org/apache/commons/csv/CSVRecord.java
>>>>>>> @@ -199,7 +199,7 @@ public class CSVRecord implements 
>>>>>>> Serializable,
>>>>>>> Iterable<String> {
>>>>>>>      public final CSVRecord immutable() {
>>>>>>>         if (isMutable()) {
>>>>>>>                 // Subclass is probably CSVMutableRecord, 
>>>>>>> freeze values
>>>>>>> -               String[] frozenValue = Arrays.copyOf(values,
>>>>>>> values.length);
>>>>>>> +               String[] frozenValue = values.clone();
>>>>>>>                 return new CSVRecord(frozenValue, mapping, 
>>>>>>> comment,
>>>>>>> recordNumber, characterPosition);
>>>>>>>         } else {
>>>>>>>                 return this;
>>>>>>> @@ -260,7 +260,7 @@ public class CSVRecord implements 
>>>>>>> Serializable,
>>>>>>> Iterable<String> {
>>>>>>>         if (isMutable()) {
>>>>>>>                 return this;
>>>>>>>         }
>>>>>>> -               String[] newValues = Arrays.copyOf(values,
>>>>>>> values.length);
>>>>>>> +               String[] newValues = values.clone();
>>>>>>>         return new CSVMutableRecord(newValues, mapping, 
>>>>>>> comment,
>>>>>>> recordNumber, characterPosition);
>>>>>>>         }
>>>>
>>>>
>>>>


---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org


Re: commons-csv git commit: CSV-216: Avoid Arrays.copyOf()

Posted by Stian Soiland-Reyes <st...@apache.org>.
String is still a type of Object (requiring GC handling of reference
counters), as you can do Object[] o = stringArray.clone(); without
casting.  (other way not so)

https://www.javaspecialists.eu/archive/Issue124.html says there is
hardly any difference, so I don't think we need to fight this one out
and can just go with the cleanest code :)

On 16 February 2018 at 11:22, sebb <se...@gmail.com> wrote:
> On 16 February 2018 at 10:01, Stian Soiland-Reyes <st...@apache.org> wrote:
>> I agree in general for .clone() on objects - and I'm trying to move
>> away from using .clone() in that Commons RDF fluent mutable/immutable
>> builder (See COMMONSRDF-49).
>>
>> But this is an Object[] - a native type.
>
> Huh?
>
> The code says String[], which is native as it's final.
>
> Object[] may not be native
>
>> I must admit I am not sure what is really the preferred way - I
>> thought .clone() is best as the VM can copy an array natively however
>> it wants, while with Arrays.copyOf it might have to check if length is
>> the same before doing anything clever.
>
> In the specific case of String[] it might be OK to use clone, but in
> general it's better to use the proper JVM methods and not try to
> second-guess how efficient they are.
>
>> On 13 February 2018 at 18:58, Gilles <gi...@harfang.homelinux.org> wrote:
>>> On Tue, 13 Feb 2018 18:40:13 +0000, sebb wrote:
>>>>
>>>> On 13 February 2018 at 09:31, Gilles <gi...@harfang.homelinux.org> wrote:
>>>>>
>>>>> On Tue, 13 Feb 2018 00:16:13 +0000 (UTC), stain@apache.org wrote:
>>>>>>
>>>>>>
>>>>>> Repository: commons-csv
>>>>>> Updated Branches:
>>>>>>   refs/heads/CSV-216 637ad2d7a -> f66a83901
>>>>>>
>>>>>>
>>>>>> CSV-216: Avoid Arrays.copyOf()
>>>>>
>>>>>
>>>>>
>>>>> Why?
>>>>
>>>>
>>>> Agreed
>>>>
>>>>>> as .clone() will do
>>>>>
>>>>>
>>>>>
>>>>> We should rather avoid using "clone()".
>>>>
>>>>
>>>> Again: why?
>>>
>>>
>>> There are many discussions about this topic on the web.[1]
>>> My point is that using "clone()" is not good advice.
>>>
>>> Gilles
>>>
>>> [1] E.g. http://vojtechruzicka.com/java-cloning-problems/
>>>
>>>
>>>> It's not clear why Arrays.copyOf(0 is considered bad, nor why clone()
>>>> is considered bad.
>>>>
>>>>
>>>>> Gilles
>>>>>
>>>>>
>>>>>> -- at least until someone tries to do
>>>>>> .withValue(x) in an out-of-range column
>>>>>>
>>>>>>
>>>>>> Project: http://git-wip-us.apache.org/repos/asf/commons-csv/repo
>>>>>> Commit:
>>>>>> http://git-wip-us.apache.org/repos/asf/commons-csv/commit/f66a8390
>>>>>> Tree: http://git-wip-us.apache.org/repos/asf/commons-csv/tree/f66a8390
>>>>>> Diff: http://git-wip-us.apache.org/repos/asf/commons-csv/diff/f66a8390
>>>>>>
>>>>>> Branch: refs/heads/CSV-216
>>>>>> Commit: f66a83901bd026369a2e8d522bd567eb2ef3f8c0
>>>>>> Parents: 637ad2d
>>>>>> Author: Stian Soiland-Reyes <st...@apache.org>
>>>>>> Authored: Fri Feb 9 16:49:51 2018 +0000
>>>>>> Committer: Stian Soiland-Reyes <st...@apache.org>
>>>>>> Committed: Tue Feb 13 00:14:52 2018 +0000
>>>>>>
>>>>>>
>>>>>>
>>>>>> ----------------------------------------------------------------------
>>>>>>  src/main/java/org/apache/commons/csv/CSVRecord.java | 4 ++--
>>>>>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>>>>>
>>>>>>
>>>>>> ----------------------------------------------------------------------
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>> http://git-wip-us.apache.org/repos/asf/commons-csv/blob/f66a8390/src/main/java/org/apache/commons/csv/CSVRecord.java
>>>>>>
>>>>>>
>>>>>> ----------------------------------------------------------------------
>>>>>> diff --git a/src/main/java/org/apache/commons/csv/CSVRecord.java
>>>>>> b/src/main/java/org/apache/commons/csv/CSVRecord.java
>>>>>> index 979119f..2be5c49 100644
>>>>>> --- a/src/main/java/org/apache/commons/csv/CSVRecord.java
>>>>>> +++ b/src/main/java/org/apache/commons/csv/CSVRecord.java
>>>>>> @@ -199,7 +199,7 @@ public class CSVRecord implements Serializable,
>>>>>> Iterable<String> {
>>>>>>      public final CSVRecord immutable() {
>>>>>>         if (isMutable()) {
>>>>>>                 // Subclass is probably CSVMutableRecord, freeze values
>>>>>> -               String[] frozenValue = Arrays.copyOf(values,
>>>>>> values.length);
>>>>>> +               String[] frozenValue = values.clone();
>>>>>>                 return new CSVRecord(frozenValue, mapping, comment,
>>>>>> recordNumber, characterPosition);
>>>>>>         } else {
>>>>>>                 return this;
>>>>>> @@ -260,7 +260,7 @@ public class CSVRecord implements Serializable,
>>>>>> Iterable<String> {
>>>>>>         if (isMutable()) {
>>>>>>                 return this;
>>>>>>         }
>>>>>> -               String[] newValues = Arrays.copyOf(values,
>>>>>> values.length);
>>>>>> +               String[] newValues = values.clone();
>>>>>>         return new CSVMutableRecord(newValues, mapping, comment,
>>>>>> recordNumber, characterPosition);
>>>>>>         }
>>>
>>>
>>>
>>> ---------------------------------------------------------------------
>>> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
>>> For additional commands, e-mail: dev-help@commons.apache.org
>>>
>>
>>
>>
>> --
>> Stian Soiland-Reyes
>> http://orcid.org/0000-0001-9842-9718
>>
>> ---------------------------------------------------------------------
>> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
>> For additional commands, e-mail: dev-help@commons.apache.org
>>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
> For additional commands, e-mail: dev-help@commons.apache.org
>



-- 
Stian Soiland-Reyes
http://orcid.org/0000-0001-9842-9718

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org


Re: commons-csv git commit: CSV-216: Avoid Arrays.copyOf()

Posted by sebb <se...@gmail.com>.
On 16 February 2018 at 10:01, Stian Soiland-Reyes <st...@apache.org> wrote:
> I agree in general for .clone() on objects - and I'm trying to move
> away from using .clone() in that Commons RDF fluent mutable/immutable
> builder (See COMMONSRDF-49).
>
> But this is an Object[] - a native type.

Huh?

The code says String[], which is native as it's final.

Object[] may not be native

> I must admit I am not sure what is really the preferred way - I
> thought .clone() is best as the VM can copy an array natively however
> it wants, while with Arrays.copyOf it might have to check if length is
> the same before doing anything clever.

In the specific case of String[] it might be OK to use clone, but in
general it's better to use the proper JVM methods and not try to
second-guess how efficient they are.

> On 13 February 2018 at 18:58, Gilles <gi...@harfang.homelinux.org> wrote:
>> On Tue, 13 Feb 2018 18:40:13 +0000, sebb wrote:
>>>
>>> On 13 February 2018 at 09:31, Gilles <gi...@harfang.homelinux.org> wrote:
>>>>
>>>> On Tue, 13 Feb 2018 00:16:13 +0000 (UTC), stain@apache.org wrote:
>>>>>
>>>>>
>>>>> Repository: commons-csv
>>>>> Updated Branches:
>>>>>   refs/heads/CSV-216 637ad2d7a -> f66a83901
>>>>>
>>>>>
>>>>> CSV-216: Avoid Arrays.copyOf()
>>>>
>>>>
>>>>
>>>> Why?
>>>
>>>
>>> Agreed
>>>
>>>>> as .clone() will do
>>>>
>>>>
>>>>
>>>> We should rather avoid using "clone()".
>>>
>>>
>>> Again: why?
>>
>>
>> There are many discussions about this topic on the web.[1]
>> My point is that using "clone()" is not good advice.
>>
>> Gilles
>>
>> [1] E.g. http://vojtechruzicka.com/java-cloning-problems/
>>
>>
>>> It's not clear why Arrays.copyOf(0 is considered bad, nor why clone()
>>> is considered bad.
>>>
>>>
>>>> Gilles
>>>>
>>>>
>>>>> -- at least until someone tries to do
>>>>> .withValue(x) in an out-of-range column
>>>>>
>>>>>
>>>>> Project: http://git-wip-us.apache.org/repos/asf/commons-csv/repo
>>>>> Commit:
>>>>> http://git-wip-us.apache.org/repos/asf/commons-csv/commit/f66a8390
>>>>> Tree: http://git-wip-us.apache.org/repos/asf/commons-csv/tree/f66a8390
>>>>> Diff: http://git-wip-us.apache.org/repos/asf/commons-csv/diff/f66a8390
>>>>>
>>>>> Branch: refs/heads/CSV-216
>>>>> Commit: f66a83901bd026369a2e8d522bd567eb2ef3f8c0
>>>>> Parents: 637ad2d
>>>>> Author: Stian Soiland-Reyes <st...@apache.org>
>>>>> Authored: Fri Feb 9 16:49:51 2018 +0000
>>>>> Committer: Stian Soiland-Reyes <st...@apache.org>
>>>>> Committed: Tue Feb 13 00:14:52 2018 +0000
>>>>>
>>>>>
>>>>>
>>>>> ----------------------------------------------------------------------
>>>>>  src/main/java/org/apache/commons/csv/CSVRecord.java | 4 ++--
>>>>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>>>>
>>>>>
>>>>> ----------------------------------------------------------------------
>>>>>
>>>>>
>>>>>
>>>>>
>>>>>
>>>>>
>>>>> http://git-wip-us.apache.org/repos/asf/commons-csv/blob/f66a8390/src/main/java/org/apache/commons/csv/CSVRecord.java
>>>>>
>>>>>
>>>>> ----------------------------------------------------------------------
>>>>> diff --git a/src/main/java/org/apache/commons/csv/CSVRecord.java
>>>>> b/src/main/java/org/apache/commons/csv/CSVRecord.java
>>>>> index 979119f..2be5c49 100644
>>>>> --- a/src/main/java/org/apache/commons/csv/CSVRecord.java
>>>>> +++ b/src/main/java/org/apache/commons/csv/CSVRecord.java
>>>>> @@ -199,7 +199,7 @@ public class CSVRecord implements Serializable,
>>>>> Iterable<String> {
>>>>>      public final CSVRecord immutable() {
>>>>>         if (isMutable()) {
>>>>>                 // Subclass is probably CSVMutableRecord, freeze values
>>>>> -               String[] frozenValue = Arrays.copyOf(values,
>>>>> values.length);
>>>>> +               String[] frozenValue = values.clone();
>>>>>                 return new CSVRecord(frozenValue, mapping, comment,
>>>>> recordNumber, characterPosition);
>>>>>         } else {
>>>>>                 return this;
>>>>> @@ -260,7 +260,7 @@ public class CSVRecord implements Serializable,
>>>>> Iterable<String> {
>>>>>         if (isMutable()) {
>>>>>                 return this;
>>>>>         }
>>>>> -               String[] newValues = Arrays.copyOf(values,
>>>>> values.length);
>>>>> +               String[] newValues = values.clone();
>>>>>         return new CSVMutableRecord(newValues, mapping, comment,
>>>>> recordNumber, characterPosition);
>>>>>         }
>>
>>
>>
>> ---------------------------------------------------------------------
>> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
>> For additional commands, e-mail: dev-help@commons.apache.org
>>
>
>
>
> --
> Stian Soiland-Reyes
> http://orcid.org/0000-0001-9842-9718
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
> For additional commands, e-mail: dev-help@commons.apache.org
>

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org


Re: commons-csv git commit: CSV-216: Avoid Arrays.copyOf()

Posted by Stian Soiland-Reyes <st...@apache.org>.
I agree in general for .clone() on objects - and I'm trying to move
away from using .clone() in that Commons RDF fluent mutable/immutable
builder (See COMMONSRDF-49).

But this is an Object[] - a native type.

I must admit I am not sure what is really the preferred way - I
thought .clone() is best as the VM can copy an array natively however
it wants, while with Arrays.copyOf it might have to check if length is
the same before doing anything clever.

On 13 February 2018 at 18:58, Gilles <gi...@harfang.homelinux.org> wrote:
> On Tue, 13 Feb 2018 18:40:13 +0000, sebb wrote:
>>
>> On 13 February 2018 at 09:31, Gilles <gi...@harfang.homelinux.org> wrote:
>>>
>>> On Tue, 13 Feb 2018 00:16:13 +0000 (UTC), stain@apache.org wrote:
>>>>
>>>>
>>>> Repository: commons-csv
>>>> Updated Branches:
>>>>   refs/heads/CSV-216 637ad2d7a -> f66a83901
>>>>
>>>>
>>>> CSV-216: Avoid Arrays.copyOf()
>>>
>>>
>>>
>>> Why?
>>
>>
>> Agreed
>>
>>>> as .clone() will do
>>>
>>>
>>>
>>> We should rather avoid using "clone()".
>>
>>
>> Again: why?
>
>
> There are many discussions about this topic on the web.[1]
> My point is that using "clone()" is not good advice.
>
> Gilles
>
> [1] E.g. http://vojtechruzicka.com/java-cloning-problems/
>
>
>> It's not clear why Arrays.copyOf(0 is considered bad, nor why clone()
>> is considered bad.
>>
>>
>>> Gilles
>>>
>>>
>>>> -- at least until someone tries to do
>>>> .withValue(x) in an out-of-range column
>>>>
>>>>
>>>> Project: http://git-wip-us.apache.org/repos/asf/commons-csv/repo
>>>> Commit:
>>>> http://git-wip-us.apache.org/repos/asf/commons-csv/commit/f66a8390
>>>> Tree: http://git-wip-us.apache.org/repos/asf/commons-csv/tree/f66a8390
>>>> Diff: http://git-wip-us.apache.org/repos/asf/commons-csv/diff/f66a8390
>>>>
>>>> Branch: refs/heads/CSV-216
>>>> Commit: f66a83901bd026369a2e8d522bd567eb2ef3f8c0
>>>> Parents: 637ad2d
>>>> Author: Stian Soiland-Reyes <st...@apache.org>
>>>> Authored: Fri Feb 9 16:49:51 2018 +0000
>>>> Committer: Stian Soiland-Reyes <st...@apache.org>
>>>> Committed: Tue Feb 13 00:14:52 2018 +0000
>>>>
>>>>
>>>>
>>>> ----------------------------------------------------------------------
>>>>  src/main/java/org/apache/commons/csv/CSVRecord.java | 4 ++--
>>>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>>>
>>>>
>>>> ----------------------------------------------------------------------
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>> http://git-wip-us.apache.org/repos/asf/commons-csv/blob/f66a8390/src/main/java/org/apache/commons/csv/CSVRecord.java
>>>>
>>>>
>>>> ----------------------------------------------------------------------
>>>> diff --git a/src/main/java/org/apache/commons/csv/CSVRecord.java
>>>> b/src/main/java/org/apache/commons/csv/CSVRecord.java
>>>> index 979119f..2be5c49 100644
>>>> --- a/src/main/java/org/apache/commons/csv/CSVRecord.java
>>>> +++ b/src/main/java/org/apache/commons/csv/CSVRecord.java
>>>> @@ -199,7 +199,7 @@ public class CSVRecord implements Serializable,
>>>> Iterable<String> {
>>>>      public final CSVRecord immutable() {
>>>>         if (isMutable()) {
>>>>                 // Subclass is probably CSVMutableRecord, freeze values
>>>> -               String[] frozenValue = Arrays.copyOf(values,
>>>> values.length);
>>>> +               String[] frozenValue = values.clone();
>>>>                 return new CSVRecord(frozenValue, mapping, comment,
>>>> recordNumber, characterPosition);
>>>>         } else {
>>>>                 return this;
>>>> @@ -260,7 +260,7 @@ public class CSVRecord implements Serializable,
>>>> Iterable<String> {
>>>>         if (isMutable()) {
>>>>                 return this;
>>>>         }
>>>> -               String[] newValues = Arrays.copyOf(values,
>>>> values.length);
>>>> +               String[] newValues = values.clone();
>>>>         return new CSVMutableRecord(newValues, mapping, comment,
>>>> recordNumber, characterPosition);
>>>>         }
>
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
> For additional commands, e-mail: dev-help@commons.apache.org
>



-- 
Stian Soiland-Reyes
http://orcid.org/0000-0001-9842-9718

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org


Re: commons-csv git commit: CSV-216: Avoid Arrays.copyOf()

Posted by Gilles <gi...@harfang.homelinux.org>.
On Tue, 13 Feb 2018 18:40:13 +0000, sebb wrote:
> On 13 February 2018 at 09:31, Gilles <gi...@harfang.homelinux.org> 
> wrote:
>> On Tue, 13 Feb 2018 00:16:13 +0000 (UTC), stain@apache.org wrote:
>>>
>>> Repository: commons-csv
>>> Updated Branches:
>>>   refs/heads/CSV-216 637ad2d7a -> f66a83901
>>>
>>>
>>> CSV-216: Avoid Arrays.copyOf()
>>
>>
>> Why?
>
> Agreed
>
>>> as .clone() will do
>>
>>
>> We should rather avoid using "clone()".
>
> Again: why?

There are many discussions about this topic on the web.[1]
My point is that using "clone()" is not good advice.

Gilles

[1] E.g. http://vojtechruzicka.com/java-cloning-problems/

> It's not clear why Arrays.copyOf(0 is considered bad, nor why clone()
> is considered bad.
>
>
>> Gilles
>>
>>
>>> -- at least until someone tries to do
>>> .withValue(x) in an out-of-range column
>>>
>>>
>>> Project: http://git-wip-us.apache.org/repos/asf/commons-csv/repo
>>> Commit: 
>>> http://git-wip-us.apache.org/repos/asf/commons-csv/commit/f66a8390
>>> Tree: 
>>> http://git-wip-us.apache.org/repos/asf/commons-csv/tree/f66a8390
>>> Diff: 
>>> http://git-wip-us.apache.org/repos/asf/commons-csv/diff/f66a8390
>>>
>>> Branch: refs/heads/CSV-216
>>> Commit: f66a83901bd026369a2e8d522bd567eb2ef3f8c0
>>> Parents: 637ad2d
>>> Author: Stian Soiland-Reyes <st...@apache.org>
>>> Authored: Fri Feb 9 16:49:51 2018 +0000
>>> Committer: Stian Soiland-Reyes <st...@apache.org>
>>> Committed: Tue Feb 13 00:14:52 2018 +0000
>>>
>>>
>>> 
>>> ----------------------------------------------------------------------
>>>  src/main/java/org/apache/commons/csv/CSVRecord.java | 4 ++--
>>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> 
>>> ----------------------------------------------------------------------
>>>
>>>
>>>
>>>
>>> 
>>> http://git-wip-us.apache.org/repos/asf/commons-csv/blob/f66a8390/src/main/java/org/apache/commons/csv/CSVRecord.java
>>>
>>> 
>>> ----------------------------------------------------------------------
>>> diff --git a/src/main/java/org/apache/commons/csv/CSVRecord.java
>>> b/src/main/java/org/apache/commons/csv/CSVRecord.java
>>> index 979119f..2be5c49 100644
>>> --- a/src/main/java/org/apache/commons/csv/CSVRecord.java
>>> +++ b/src/main/java/org/apache/commons/csv/CSVRecord.java
>>> @@ -199,7 +199,7 @@ public class CSVRecord implements Serializable,
>>> Iterable<String> {
>>>      public final CSVRecord immutable() {
>>>         if (isMutable()) {
>>>                 // Subclass is probably CSVMutableRecord, freeze 
>>> values
>>> -               String[] frozenValue = Arrays.copyOf(values,
>>> values.length);
>>> +               String[] frozenValue = values.clone();
>>>                 return new CSVRecord(frozenValue, mapping, comment,
>>> recordNumber, characterPosition);
>>>         } else {
>>>                 return this;
>>> @@ -260,7 +260,7 @@ public class CSVRecord implements Serializable,
>>> Iterable<String> {
>>>         if (isMutable()) {
>>>                 return this;
>>>         }
>>> -               String[] newValues = Arrays.copyOf(values, 
>>> values.length);
>>> +               String[] newValues = values.clone();
>>>         return new CSVMutableRecord(newValues, mapping, comment,
>>> recordNumber, characterPosition);
>>>         }


---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org


Re: commons-csv git commit: CSV-216: Avoid Arrays.copyOf()

Posted by sebb <se...@gmail.com>.
On 13 February 2018 at 09:31, Gilles <gi...@harfang.homelinux.org> wrote:
> On Tue, 13 Feb 2018 00:16:13 +0000 (UTC), stain@apache.org wrote:
>>
>> Repository: commons-csv
>> Updated Branches:
>>   refs/heads/CSV-216 637ad2d7a -> f66a83901
>>
>>
>> CSV-216: Avoid Arrays.copyOf()
>
>
> Why?

Agreed

>> as .clone() will do
>
>
> We should rather avoid using "clone()".

Again: why?

It's not clear why Arrays.copyOf(0 is considered bad, nor why clone()
is considered bad.

> Gilles
>
>
>> -- at least until someone tries to do
>> .withValue(x) in an out-of-range column
>>
>>
>> Project: http://git-wip-us.apache.org/repos/asf/commons-csv/repo
>> Commit: http://git-wip-us.apache.org/repos/asf/commons-csv/commit/f66a8390
>> Tree: http://git-wip-us.apache.org/repos/asf/commons-csv/tree/f66a8390
>> Diff: http://git-wip-us.apache.org/repos/asf/commons-csv/diff/f66a8390
>>
>> Branch: refs/heads/CSV-216
>> Commit: f66a83901bd026369a2e8d522bd567eb2ef3f8c0
>> Parents: 637ad2d
>> Author: Stian Soiland-Reyes <st...@apache.org>
>> Authored: Fri Feb 9 16:49:51 2018 +0000
>> Committer: Stian Soiland-Reyes <st...@apache.org>
>> Committed: Tue Feb 13 00:14:52 2018 +0000
>>
>>
>> ----------------------------------------------------------------------
>>  src/main/java/org/apache/commons/csv/CSVRecord.java | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> ----------------------------------------------------------------------
>>
>>
>>
>>
>> http://git-wip-us.apache.org/repos/asf/commons-csv/blob/f66a8390/src/main/java/org/apache/commons/csv/CSVRecord.java
>>
>> ----------------------------------------------------------------------
>> diff --git a/src/main/java/org/apache/commons/csv/CSVRecord.java
>> b/src/main/java/org/apache/commons/csv/CSVRecord.java
>> index 979119f..2be5c49 100644
>> --- a/src/main/java/org/apache/commons/csv/CSVRecord.java
>> +++ b/src/main/java/org/apache/commons/csv/CSVRecord.java
>> @@ -199,7 +199,7 @@ public class CSVRecord implements Serializable,
>> Iterable<String> {
>>      public final CSVRecord immutable() {
>>         if (isMutable()) {
>>                 // Subclass is probably CSVMutableRecord, freeze values
>> -               String[] frozenValue = Arrays.copyOf(values,
>> values.length);
>> +               String[] frozenValue = values.clone();
>>                 return new CSVRecord(frozenValue, mapping, comment,
>> recordNumber, characterPosition);
>>         } else {
>>                 return this;
>> @@ -260,7 +260,7 @@ public class CSVRecord implements Serializable,
>> Iterable<String> {
>>         if (isMutable()) {
>>                 return this;
>>         }
>> -               String[] newValues = Arrays.copyOf(values, values.length);
>> +               String[] newValues = values.clone();
>>         return new CSVMutableRecord(newValues, mapping, comment,
>> recordNumber, characterPosition);
>>         }
>
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
> For additional commands, e-mail: dev-help@commons.apache.org
>

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org