On Sun, Mar 28, 2010 at 5:53 PM, Lang Yang <[email protected]> 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 <[email protected]> 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 <[email protected]> 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 <[email protected]> >> 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 <[email protected]> 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 >> >> > >> >> >> > >> >
