Community
Participate
Working Groups
I20070109-1805 JavaCore.newLibraryEntry(.., IClasspathAttribute[], ..) should throw an exception if the given IClasspathAttribute[] is not null. Currently, null is silently accepted and leads to NPEs when the extra attributes are accessed later. Although unspecified, passing null for accessRules is currently accepted (and explicitly handled). Therefore, it's maybe better to continue allowing null for accessRules. Even jdt.core code currently relies on this behavior, see ClasspathEntry.decodeAccessRules(NodeList). I just fixed a broken invocation in our class BuildPathSupport, but such problems may be hard to find in general since creation and usage are quite separated.
I guess you meant that it should throw an exception if the extraAttributes argument is null. To be investigated for 3.4.
> I guess you meant that it should throw an exception if the extraAttributes > argument is null. Yes, sorry about that. You should actually fix this for all 5 callers of the ClasspathEntry constructor in JavaCore. And for the accessRules parameter, you should either explicitly allow null in the Javadoc or throw an exception as well.
Deferring post 3.4
This one should be trivial. I think IllegalArgumentException should be thrown if null is being passed and this is specified in the Javadoc.
(In reply to comment #4) > This one should be trivial. I think IllegalArgumentException should be thrown > if null is being passed and this is specified in the Javadoc. It would appear more consistent to throw ClasspathEntry#AssertionFailedException as is done by org.eclipse.jdt.core.JavaCore.newSourceEntry(IPath, IPath[], IPath[], IPath, IClasspathAttribute[]) ?
Jerome, comment#5 was addressed to you for input.
(In reply to comment #6) > Jerome, comment#5 was addressed to you for input. > Yes, using ClasspathEntry#AssertionFailedException makes sense
Created attachment 134940 [details] Proposed patch + tests Jerome, please review.
After more thought, I'm wondering why we would not allow extraAttributes to be null and consider that this is the same as an empty list. This would be more consistent with the fact that accessRules can also be null. So in both cases, we could specify that null is accepted and fix the implementation for extraAttributes. Any thought?
> After more thought, I'm wondering why we would not allow extraAttributes to be > null and consider that this is the same as an empty list. Would also be fine for me, but then I would do this for all parameters that accept an empty array (i.e. also for inclusionPatterns and exclusionPatterns).
Created attachment 135317 [details] Revised patch Jerome, please review, thanks.
(In reply to comment #11) > Created an attachment (id=135317) [details] > Revised patch > Jerome, please review, thanks. BTW, this patch would "allow extraAttributes to be null and consider that this is the same as an empty list". Likewise for accessRules, inclusionPatterns and exclusionPatterns. I left the javadoc as it is: i.e did not update it to say "empty list or null.". There are many more methods whose javadoc would have to be updated than are involved in the code change and I didn't see the value in the exercise. (the real value being not allowing null references to be propagated and captured in the state associated with ClasspathEntry, thereby leading to an NPE downstream resulting in problems that "may be hard to find in general since creation and usage are quite separated." per comment #0 - this is accomplished by the code change)
Comment on attachment 135317 [details] Revised patch Patch looks good
Released in HEAD for 3.5 RC1
Verified for 3.5RC1 using I20090514-2000
