Checking in src/org/openide/util/lookup/SimpleLookup.java;
/shared/data/ccvs/repository/openide/util/src/org/openide/util/lookup/SimpleLookup.java,v
 <--  SimpleLookup.java
new revision: 1.2; previous revision: 1.1
done
Checking in test/unit/src/org/openide/util/lookup/SimpleLookupTest.java;
/shared/data/ccvs/repository/openide/util/test/unit/src/org/openide/util/lookup/SimpleLookupTest.java,v
 <--  SimpleLookupTest.java
new revision: 1.2; previous revision: 1.1
done

Good commit, thanks.  

I think this should be reverted. The previous Javadoc did *not* permit clients
to pass a null array element. It explicitly forbade a null array, which is
redundant since the general contract for *all* NB APIs is that nulls are always
forbidden unless explicitly permitted; it implicitly forbade a null array
element by the same rule.

Passing a null in the array makes it seem as if the Lookup is acting like a
Collection which permits null values, which is not true - the null will not be
returned from e.g. l.lookup(new Lookup.Template(Object.class)).allInstances().

I don't have any strong feeling about it, just FYI.

> the general contract for *all* NB APIs is that nulls are always
> forbidden unless explicitly permitted

I have always problem with it. It's API implementor friendly. API client
friendly approach is opposite:
  - all methods accept null arguments until stated
  - no method return null until stated

Lookups.fixed() contructs utility Lookup so I suppose main intent is to remove
load from clients. The change supports this intent.

It's not about implementor vs. client, it's about complexity of the API
definition. Permitting nulls (if it were properly documented, which this change
was not) adds another clause to the spec of the method: "oh, and also, if you
put some nulls into the array, they will be accepted - but will not affect the
lookup content in any way". Which is gratuitous, IMHO.

Just a note: until now, if a client passed null to Lookups.fixed() by accident
(typically getSomething() without noticing the result may be null), client was
warned quickly. Now, the client may spend hours trying to find out why a given
object was not found in the lookup, without any clue that it was simply
discarded by the system.

But, regardless if the current or the original state will persist, I think that
the behaviour should be explicitely described in the javadoc (in particular,
what will happen if new Object[] {null} is put into Lookups.fixed.).

Agreed. Reopening to at least have Javadoc updated to mention the new semantics
(technically apichanges.xml should mention it too).

IMO the meaning of Javadoc is that Lookups.fixed(null); will throw NPE.

I agree w/ Jesse that adding nulls to any lookup is not expected and noone
declared that lookup implementations should filter them. Now you introduced
inconsistency between Lookup fixed(Object[] objectsToLookup) and Lookup
fixed(Object[] objectsToLookup, , InstanceContent.Convertor convertor) or
Lookups.singleton(Object objectToLookup).

Re API friendly to clients or implementors: current policy is set and works
well. Hybrid approach would be just a source of confusion. BTW: now we can check
our rules using assertions and have faster production code. If we allow nulls
generaly it will no longer be possible. 

Petr, please try to address comments of the reviewers. If you have not manage 
to satisfy them by Thursday, I'll rollback your changes. Also, next time 
please do not commit API changes without fast track review. 

Petr, do not you want to fix the open issues? 

I have expressed my opinion. Leaving on maintainer to accept/reject.

Well, you have been asked to "do something" not express your opinion. If that 
is all you are going to do, it is simpler to revert then "do something" 
myself. Tomorrow is the deadline. 

I have updated my clients. Maintainer can either:
  - accept (+ fixing Javadoc)
  - or reject (+ adding additional null checks and fixing Javadoc)

without breaking these clients.

With a sadness in heart I've just rollback Petr's commit which I otherwise 
liked.  
 
The reason is that there were objections from the reviewers that Petr refused 
to address and it is not my interest to address them either. 
 
I'd like to remind Petr that next time he should use fast track review to get 
the review *before* integration of a change, so I do not have spend time on 
reverting his commits that were found unsound by majority of reviewers. 
 
 
 
 
"Reverting back Petr's integration of 68079 as it has been found 
unappropriate/unfinished by majority of the reviewers" 
Checking in src/org/openide/util/lookup/SimpleLookup.java; 
/cvs/openide/util/src/org/openide/util/lookup/SimpleLookup.java,v  <--  
SimpleLookup.java 
new revision: 1.3; previous revision: 1.2 
done 
Checking in test/unit/src/org/openide/util/lookup/SimpleLookupTest.java; 
/cvs/openide/util/test/unit/src/org/openide/util/lookup/SimpleLookupTest.java,v  
<--  SimpleLookupTest.java 
new revision: 1.3; previous revision: 1.2 
 

I think we should:
-document that null elements in the "fixed" array are prohibited.
-do a precondition testing (something like
Arrays.asList(objectsToLookup).contains(null)=>throw new NPE()).
-test this behavior.

If there are no objections and unless someone else wants to do it, I will do
these changes.

I was not quite clear. I not only rollbacked the changes, but also added a 
test. Additions to javadoc are always welcomed. 

AWT uses IllegalArgumentException when invalid argument is passed to method
(including null). I think this is better than NPE as NPE might be thrown also
due to other bug in code. On the other hand NPE is more specific.