Re: [imageio] return a String array

[Nathan Beyer <nd...@apache.org>]

On Sun, Mar 28, 2010 at 5:53 PM, Lang Yang <ya...@gmail.com> wrote:
> Thanks, I will do so. Those variables are defined as "protected" in the
> classes since we want those variables to be hidden. If we only return the
> references to those variables, then users could access to them "publicly".
>
> I also found that there few constructors don't clone mutable objects. like
> this one:
>
> package: javax.imageio.spi;
>
>    public ImageReaderSpi(String vendorName, String version, String[] names,
> String[] suffixes,
>                             String[] MIMETypes, String pluginClassName,
>                             Class[] inputTypes, String[] writerSpiNames,
>                             boolean supportsStandardStreamMetadataFormat,
>                             String nativeStreamMetadataFormatName,
>                             String nativeStreamMetadataFormatClassName,
>                             String[] extraStreamMetadataFormatNames,
>                             String[] extraStreamMetadataFormatClassNames,
>                             boolean supportsStandardImageMetadataFormat,
>                             String nativeImageMetadataFormatName,
>                             String nativeImageMetadataFormatClassName,
>                             String[] extraImageMetadataFormatNames,
>                             String[] extraImageMetadataFormatClassNames) {
>        super(vendorName, version, names, suffixes, MIMETypes,
> pluginClassName,
>                supportsStandardStreamMetadataFormat,
> nativeStreamMetadataFormatName,
>                nativeStreamMetadataFormatClassName,
> extraStreamMetadataFormatNames,
>                extraStreamMetadataFormatClassNames,
> supportsStandardImageMetadataFormat,
>                nativeImageMetadataFormatName,
> nativeImageMetadataFormatClassName,
>                extraImageMetadataFormatNames,
> extraImageMetadataFormatClassNames);
>
>        if (inputTypes == null || inputTypes.length == 0) {
>            throw new
> NullPointerException(Messages.getString("imageio.5C"));
>        }
>        this.inputTypes = inputTypes;
>        this.writerSpiNames = writerSpiNames;
>    }
>
> should we also change the code to:
>
>        this.inputTypes = (inputTypes == null ? null : inputTypes.clone());
>        this.writerSpiNames = (writerSpiNames ==  null ? null :
> writerSpiNames.clone());
>
> Is there an efficiency consideration for not cloning these objects?

Yes, the constructor should clone the array in this case.

As for performance considerations - it does cost more, but it's
trivial, especially considering that in most cases, the array sizes
are very small (less than 15 entries). It's generally not a
significant amount of data being cloned.

>
> Thanks,
>
> Lang
>
> On Sun, Mar 28, 2010 at 2:16 PM, Nathan Beyer <nd...@apache.org> wrote:
>
>> The javadoc for those methods isn't explicit about it being immutable,
>> but I always assume in these cases, the data is a copy, because it's
>> uncommon to leave these values "live" and that behavior is generally
>> documented as such.
>>
>> So yet, it should probably be fixed.
>>
>> On Sun, Mar 28, 2010 at 11:42 AM, Yang Lang <ya...@gmail.com> wrote:
>> > Thanks Nathan, that’s really helpful.
>> >
>> > Both of them are in public classes.
>> > javax.imageio.spi.ImageReaderWriterSpi.getFormatNames()
>> > javax.imageio.spi.ImageWriterSpi.getImageReaderSpiNames()
>> >
>> > That’s why I was confused. In order to prevent the array be manipulated,
>> > shouldn’t we always clone it in public APIs? There are few other methods
>> in
>> > javax package returns string[] without cloning. Can I assume this is a
>> bug
>> > and create a JIRA/patch for it?
>> >
>> > Thanks,
>> >
>> > Lang
>> >
>> > On Sun, Mar 28, 2010 at 1:47 AM, Nathan Beyer <nd...@apache.org>
>> wrote:
>> >
>> >> What's the context of each class? Is one class public (javax.*) and
>> >> the other an internal class (org.apache.harmony.*)?
>> >>
>> >> This isn't something that's unique to String arrays, nor arrays;
>> >> returning a copy of a field is a safety measure to ensure
>> >> immutability, thread safety and other properties that an API may want
>> >> to guarantee. Frequently, a public API will define certain classes as
>> >> immutable, so the cloning of arrays is necessary, as the array's
>> >> contents could be manipulated -- an array is not immutable.
>> >>
>> >> -Nathan
>> >>
>> >>
>> >> On Sat, Mar 27, 2010 at 10:20 PM, Yang Lang <ya...@gmail.com> wrote:
>> >> > Hi guys,
>> >> >
>> >> > When I am reading through ImageIO package’s source code, I found out
>> >> there
>> >> > are two difference way to return a String[].
>> >> >
>> >> > For some methods, they call Arrays.clone() to clone a new string[] and
>> >> > return the new one while some other methods returning the original
>> >> String[]
>> >> > directly.
>> >> >
>> >> > e.g.:
>> >> > 1.
>> >> > public String[] getFormatNames() {
>> >> >    return names.clone();
>> >> > }
>> >> >
>> >> > 2.
>> >> > public String[] getImageReaderSpiNames() {
>> >> >    return readerSpiNames;
>> >> > }
>> >> >
>> >> > I am wondering what the difference between these two usages is. For
>> what
>> >> > kind of situations I need to clone a new array ?
>> >> >
>> >> > Thanks,
>> >> >
>> >> > Lang
>> >> >
>> >>
>> >
>>
>