Bug 41166 - [2004-06-07] Get ready for 1.5 compilation - the enum keyword
[2004-06-07] Get ready for 1.5 compilation - the enum keyword
Status: RESOLVED FIXED
Product: platform
Classification: Unclassified
Component: -- Other --
4.x
All All
: P2 (vote)
: 4.x
Assigned To: Jaroslav Tulach
issues@platform
core-promod
: API_REVIEW_FAST
Depends on: 44467
Blocks:
  Show dependency treegraph
 
Reported: 2004-03-20 07:34 UTC by Jaroslav Tulach
Modified: 2008-12-23 00:34 UTC (History)
4 users (show)

See Also:
Issue Type: TASK
:


Attachments
Sample introduction of org.openide.util.Enumerations class that provides the same functionality as enum package, as verified by tests (28.77 KB, patch)
2004-03-20 08:06 UTC, Jaroslav Tulach
Details | Diff
A version with full javadoc, yet without apichanges (42.34 KB, patch)
2004-05-19 15:02 UTC, Jaroslav Tulach
Details | Diff
Nearly final diff - includes changes in most of modules in CVS (249.93 KB, patch)
2004-06-03 15:49 UTC, Jaroslav Tulach
Details | Diff
Commit log (26.51 KB, text/plain)
2004-06-07 09:29 UTC, Jaroslav Tulach
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Jaroslav Tulach 2004-03-20 07:34:11 UTC
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.
Comment 1 Jaroslav Tulach 2004-03-20 08:06:47 UTC
Created attachment 14063 [details]
Sample introduction of org.openide.util.Enumerations class that provides the same functionality as enum package, as verified by tests
Comment 2 Jaroslav Tulach 2004-03-20 08:08:52 UTC
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.
Comment 3 _ ttran 2004-03-22 20:59:31 UTC
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?
Comment 4 Jesse Glick 2004-03-23 01:19:37 UTC
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.
Comment 5 Jesse Glick 2004-03-23 01:20:36 UTC
(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.)
Comment 6 Jaroslav Tulach 2004-03-23 08:35:32 UTC
"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.
Comment 7 Jaroslav Tulach 2004-05-19 10:16:15 UTC
Ok, I will try to improve the documentation and come back for another
review when I found some time.
Comment 8 Jaroslav Tulach 2004-05-19 15:02:09 UTC
Created attachment 15005 [details]
A version with full javadoc, yet without apichanges
Comment 9 Jaroslav Tulach 2004-05-19 15:04:40 UTC
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. 
Comment 10 Jesse Glick 2004-05-19 15:53:19 UTC
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. :-)
Comment 11 Jaroslav Tulach 2004-05-21 07:34:57 UTC
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
Comment 12 Jaroslav Tulach 2004-05-31 07:04:56 UTC
I'll work on this a bit more and try to commit the change this week.
Comment 13 Jaroslav Tulach 2004-06-03 15:49:06 UTC
Created attachment 15462 [details]
Nearly final diff - includes changes in most of modules in CVS
Comment 14 Jaroslav Tulach 2004-06-03 15:55:41 UTC
* 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
Comment 15 Jesse Glick 2004-06-03 16:35:59 UTC
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.
Comment 16 Jaroslav Tulach 2004-06-07 09:29:19 UTC
Created attachment 15509 [details]
Commit log
Comment 17 Jaroslav Tulach 2004-06-07 09:29:51 UTC
openide rev. 4.37


By use of this website, you agree to the NetBeans Policies and Terms of Use. © 2012, Oracle Corporation and/or its affiliates. Sponsored by Oracle logo