On 8/30/07, Maarten Coene <[EMAIL PROTECTED]> wrote:
>
> I have a junit test to reproduce the NPE :-)
Great!
My problem was how it could be solved. I understand why you've added the
> for-loop, but I'm not sure we should keep it:
> - I don't think similar checks are made in other Ivy methods which accept
> a collection of some type
Indeed, but I don't think it's a good reason to remove it
- it gives you a false feeling of safety: the following code will cause a
> CCE as well later in the code:
You're right, and it's a good reason to remove it :-)
EvictionData data = new EvictionData("default", null, null, new
> ArrayList());
> data.getSelected().add(new Object());
>
> I would prefer removing the for-loop and adding javadoc to the constructor
> (and the markEvicted methods, where this EvictionData is used). And maybe we
> should implement a consistent way of verifying the content of collections of
> all public methods (and also a way to make sure they can never contain
> objects of the wrong type, like the example above illustrates, for instance
> by using something similar like
> org.apache.commons.collections.collection.TypedCollection as fields).
Ok, I'm fine with removing the check and updating the doc. Using sg like
TypedCollection may be a good option, but I don't want to introduce a
dependency on commons-collections and neither reinvent the wheel... So maybe
we can stick with documentation only checks. WDYT?
Xavier
Maarten
>
> ----- Original Message ----
> From: Xavier Hanin <[EMAIL PROTECTED]>
> To: [email protected]
> Sent: Thursday, August 30, 2007 2:39:18 PM
> Subject: Re: IVY-590
>
> On 8/30/07, Maarten Coene <[EMAIL PROTECTED]> wrote:
> >
> > Hi,
> >
> > jira issue IVY-590 reports a NullPointerException which is caused by the
> > following code (selected can be null):
> >
> > public EvictionData(String rootModuleConf, IvyNode parent,
> > ConflictManager conflictManager,
> > Collection selected) {
> > this.rootModuleConf = rootModuleConf;
> > this.parent = parent;
> > this.conflictManager = conflictManager;
> > this.selected = selected;
> > for (Iterator iter = selected.iterator(); iter.hasNext();) {
> > Object o = (Object) iter.next();
> > if (!(o instanceof IvyNode)) {
> > throw new IllegalArgumentException(
> > "selected nodes must be instance of IvyNode.
> > Found: "
> > + o.getClass().getName());
> > }
> > }
> > }
> >
> >
> > Several patches have been contributed to fix this NPE by checking the
> > selected parameter for null before entering the for-loop. However, I
> would
> > like to remove this for loop from the code, since the collection always
> > contains IvyNodes.
> >
> > Does someone has an idea why this instanceof-check has been added there?
>
>
> I think I introduced this check because it happened once that we had a non
> IvyNode instance in the collection, causing a CCE later which was
> difficult
> to diagnose. Since this constructor is public, making a check seems like a
> not too bad idea (if it was done correctly, ie by checking null before
> :-)).
> But if you experience a performance issue with this check, we can remove
> the
> check, and add a comment in the constructor saying that the collection
> must
> contain only IvyNode, otherwise a CCE may be cast later in other sections
> of
> the code.
>
> To fix the bug, I thought it would be better to first find the case when
> the
> collection is null and add a junit test for it. But it may take more time
> than what we have.
>
> WDYT?
>
> Xavier
>
> regards,
> > Maarten
> >
> >
> >
> >
> >
> >
> >
> ____________________________________________________________________________________
> > Be a better Globetrotter. Get better travel answers from someone who
> > knows. Yahoo! Answers - Check it out.
> > http://answers.yahoo.com/dir/?link=list&sid=396545469
> >
>
>
>
> --
> Xavier Hanin - Independent Java Consultant
> http://xhab.blogspot.com/
> http://incubator.apache.org/ivy/
> http://www.xoocode.org/
>
>
>
>
>
>
>
> ____________________________________________________________________________________
> Yahoo! oneSearch: Finally, mobile search
> that gives answers, not web links.
> http://mobile.yahoo.com/mobileweb/onesearch?refer=1ONXIC
>
--
Xavier Hanin - Independent Java Consultant
http://xhab.blogspot.com/
http://incubator.apache.org/ivy/
http://www.xoocode.org/
