This Bugzilla instance is a read-only archive of historic NetBeans bug reports. To report a bug in NetBeans please follow the project's instructions for reporting issues.
The "enum" is keyword in 1.5 and that is why we have to get rid of org.openide.util.enum package as nobody who would refer to it could compile.
Created attachment 14063 [details] Sample introduction of org.openide.util.Enumerations class that provides the same functionality as enum package, as verified by tests
I'd like to ask for opinion and review. First question is whether we want to compile with 1.5, second whether the Enumerations class is the right solution.
third, shouldn't we deprecate this package wholesale and use Collections instead? How much is it used? Where? Anything unique inorg.openide.util.enum which we can't have in Collections?
Prefer to replace public static Enumeration EMPTY; with public static Enumeration empty(); as it could avoid initializing this if it is not used. Not very important. HashSetWithInverseContains looks very scary to me. Could you please reformat the class? It seems to use multiple distinct formatting conventions completely at random... Of course Javadoc is completely missing here. Trung - a fair amount of code does still use Enumeration. We could get rid of it all but it would take a while, I think. Some Java APIs still require it - e.g. ProxyClassLoader is required to produce an Enumeration<URL> from getAllResources(String), and it is much simpler to use e.g. SequenceEnumeration (now Enumerations.concat I guess) than to write it from scratch. I have no idea how useful convert, filter, process, etc. are; I think I have only used concat myself.
(BTW I *don't* think we should compile with -source 1.5, but I think it is wise to get rid of 'enum' as an identifier as soon as possible anyway.)
"third" == "whether Enumeration is the right solution". The reason why there is no javadoc and the formating may not be correct is that I did not want to waste my time with it, as it might turn out that we do not need or want that class at all. Jesse is right, Enumeration is sometimes useful and our support is not completely superseded by collections. During reimplementation of the class I have removed some of the original NetBeans only implementations like ArrayEnumeration and replaced that by a call to Collections. Still there are things that are not covered by anything existing in JDK. As was pointed out SequenceEnumeration, FilterEnumeration, QueueEnumeration simplify creation of "lazy" enumerations which would be otherwise very likely replaced by ineffective "put everything into ArrayList and turn into enumeration". These classes are used on bunch of places including new code like looks and masterfs and it would be a bit dangerous to write them everytime from scratch. Maintenance of these classes should not be problem at all. There has not been a bug in these classes for ages and test coverage is 99% according to measurements done year ago. If we agree that Enumerations class is the way to go, I will create polished version with all api attributes - javadoc, api changes, etc.
Ok, I will try to improve the documentation and come back for another review when I found some time.
Created attachment 15005 [details] A version with full javadoc, yet without apichanges
I have provided javadoc, addressed some of Jesse's comments and replaced Set and Map with a special interface Processor. Seems to be more clearer to me now, but I can change it. Review opinions welcomed.
In removeDuplicates, I think a HashSet would be correct, not a WeakSet. Consider that you have a sequence of {X@12[foo], X@34[bar], ...<unrelated x 10000>..., X@56[foo]} where X.equals/hashCode is based on its String field - common pattern I think. Now imagine that the input Enumeration generates its elements on the fly (does not hold refs to them) and that the consumer does a bunch of work in between elements it receives. It could happen that X@12[foo] is sent through the filter and added to the WeakSet; returned to the consumer; used; discarded; some hundreds of other elements go through; the dead X@12 is collected; and then later X@56[foo] comes through. With the current code, X@56 will be passed to the consumer, even though X@56.equals(X@12) (or would, if X@12 still existed). This is incorrect I think. If any client actually wishes to use removeDuplicates but permit earlier elements to be collected, IMHO they would better write the code directly to keep control over how it works. Typical usage for this method anyway involves a smallish number of elements, so holding hard refs to all the unique elements does not seem a problem to me. Javadoc for convert(...) has a typo: s/Process/Processor/. Similarly in some other methods. Also statement should end in ';' not ':'. Avoid abbreviations like "btw" and "thru" in Javadoc. Typo s/Onldy/Only/g in ToAdd methods. Also IllegalStateException is not right; use UnsupportedOperationException. Minor English usage: "JDK1.5 treats enum as keyword and that is why this class had to be replaced by <method>" is awkward. Prefer e.g. "Use <method> instead of this class. This package is deprecated due to a naming clash with JDK 1.5's 'enum' keyword." JUnit tests don't need main() methods, just noise. Otherwise, all looks good. Looking forward to being able to rebuild modules under JDK 1.5 without getting warnings about o.o.util.enum usages. :-)
Thanks for review Jesse, will implement your comments. Especially thanks for the WeakSet, I wanted to keep the memory low and thought just about the == relation which would work perfectly with WeakSet, but of course equal is different. Of course when integrating I will try to make sure that no code in NetBeans standard build is using the old package. Re: testcases do not need main - then tell me how to debug them!? As you may know we are not able to debug openide tests in 4.0 and I have to use 3.6
I'll work on this a bit more and try to commit the change this week.
Created attachment 15462 [details] Nearly final diff - includes changes in most of modules in CVS
* In removeDuplicates, I think a HashSet would be correct, not a * WeakSet. Consider that you have a sequence of Tested and changed. * Javadoc for convert(...) has a typo: s/Process/Processor/. Similarly Changed all I could find. * in some other methods. Also statement should end in ';' not ':'. Not sure about that. * Avoid abbreviations like "btw" and "thru" in Javadoc. Replaced by through, as I am unsure of spelling I'd rather use thru. * Typo s/Onldy/Only/g in ToAdd methods. Ok. * Also IllegalStateException is * not right; use UnsupportedOperationException. Well not all methods can be unsupported. Only optional one. For example throwing UOE from isEmpty is definitively illegal. So I think ISE is fine. Can change however. * Minor English usage: "JDK1.5 treats enum as keyword and that is why Not yet. So what needs to be done: * maybe ISE -> UOE * change following --- Minor English usage: "JDK1.5 treats enum as keyword and that is why this class had to be replaced by <method>" is awkward. Prefer e.g. "Use <method> instead of this class. This package is deprecated due to a naming clash with JDK 1.5's 'enum' keyword." --- * increase spec number of openide * write apichanges * make all modified modules depend on that number
Re. "Well not all methods can be unsupported. Only optional one. For example throwing UOE from isEmpty is definitively illegal. So I think ISE is fine." - still disagree. ISE is definitely wrong - you are not in an illegal state, you just can't call that method. Perhaps UOE is not supposed to be thrown from isEmpty, but ISE isn't either; at least UOE conveys the correct intention. Re. colon problem: still broken, e.g. in + * Example to convert any objects into strings: + * <pre> + * Processor convertToString = new Process () { + * public Object process (Object obj, Collection toAdd) { // toAdd is always null + * return obj.toString (): // converts to string + * } + * }; + * Enumeration strings = Enumerations.convert (elems, convertToString); + * </pre> and elsewhere. And you did not fix s/Process/Processor/ here, or in fact in several other places. Check it again. Also generally in Javadoc comments, minor thing: sample code does not follow java.sun.com formatting conventions esp. w.r.t. spaces before parens, and sometimes uses 3-space indentation rather than 4. "through" is correct, check dictionary.com in general.
Created attachment 15509 [details] Commit log
openide rev. 4.37