I accepted the original criticism that going Enumeration -> List -> Stream has an overhead and I tried to address that by using a decorator. I believe that Streams API can at least implement the logic run by an original Enumeration in a more concise way, or provide more powerful idioms. That IMO makes it worth the while to investigate the Streams alternatives.
In the process I also found out that other iterators could be used in a more efficient way, or that there is a redundant object construction going on. If you wish, I may spend some time to describe the changes I introduced, and maybe then we could discuss the their merits. Gintas 2018-05-18 8:30 GMT+02:00 Jaikiran Pai <jai.forums2...@gmail.com>: > If your objection is that I claimed that these qualify as "most of the > cases" - I really don't know what else to say then. The original commit > which did this change is this[1]. I haven't reviewed it fully, but the very > first few changes that are done are these [2] [3] [4] [5][6]. > > Of course, there's a subsequent commit which then uses a different new > util, instead of just using the existing iterator/enumeration. Speaking of > the subsequent commit, it still doesn't undo the (IMO unnecessary) change > that was done to some of the code (take a look at > ArgumentProcessorRegistry.java for example). > > Even if these don't fall under "most of the cases", why even change these > places? I'm sure you would know this - the Enumeration or APIs that you > refactored aren't even deprecated in Java version 10. > > Anyway, I'm really getting tired of these back and forth arguments on > refactoring. The reason I get involved in certain open source projects is > to get a chance to work with like minded developers and learn something out > of it and not to go wage a war on which coding style is better or try and > be critical of other committers' commits. Unfortunately, in the recent > past, this has reached a state where I have ended up spending more time > being critical of changes that have gone in, than actually adding much code > of value. As much as I try to stay away from reviews or checking the commit > logs, I just keep going back to them. I don't want to end up being a grumpy > guy criticizing the commits. I'm just going to take a break from this for > some days and be a regular user and come back and see if I still enjoy > contributing. > > [1] https://github.com/apache/ant/commit/070c3bc86f85e8f01cb624f > e50ae82f0d11171b2 > > [2] https://github.com/apache/ant/commit/070c3bc86f85e8f01cb624f > e50ae82f0d11171b2#diff-21eb59eaf9f2b5d0b487aeb5e5022ccdL746 > [3] https://github.com/apache/ant/commit/070c3bc86f85e8f01cb624f > e50ae82f0d11171b2#diff-21eb59eaf9f2b5d0b487aeb5e5022ccdL834 > [4] https://github.com/apache/ant/commit/070c3bc86f85e8f01cb624f > e50ae82f0d11171b2#diff-21eb59eaf9f2b5d0b487aeb5e5022ccdL888 > [5] https://github.com/apache/ant/commit/070c3bc86f85e8f01cb624f > e50ae82f0d11171b2#diff-21eb59eaf9f2b5d0b487aeb5e5022ccdL1359 > > [6] https://github.com/apache/ant/commit/070c3bc86f85e8f01cb624f > e50ae82f0d11171b2#diff-b98a3d2097d6a9b5d7e0fc2eac033f24L348 > > > -Jaikiran > > > > On 18/05/18 11:15 AM, Gintautas Grigelionis wrote: > >> I'm not quite sure that what you say was true "in most of the cases". >> >> Gintas >> >> 2018-05-18 6:52 GMT+02:00 Jaikiran Pai <jai.forums2...@gmail.com>: >> >> To be honest, I don't think this deprecation/conversion change is >>> good(including this recent commit whichintroduces a >>> StreamUtils.enumerationAsStream(...)) >>> >>> To give you an example, the code that was previously present would (in >>> most of the cases) get hold of an enumeration and would iterate over it >>> and >>> during that iteration would runcertain logic. With the changes that went >>> in >>> (which again is a bulk change!) the code now gets hold of an enumeration >>> and instead of just getting on the business of iterating and running >>> certain logic, instead now passes this enumeration aroundto convert it >>> into >>> some other form (thus additional code plus additional objects allocated >>> in >>> the process), all this to iterate over it and run some logic on it - all >>> of >>> which was already possible with the enumeration that was already >>> available. >>> >>> >>> -Jaikiran >>> >>> >>> >>> On 18/05/18 12:22 AM, Gintautas Grigelionis wrote: >>> >>> Thanks for reviewing, I hope Spliterators will do a better job. >>>> >>>> Gintas >>>> >>>> 2018-05-17 8:37 GMT+02:00 Jaikiran Pai <jai.forums2...@gmail.com>: >>>> >>>> I agree. Especially when it's being done on something like for archive >>>> >>>>> entries which can betoo many depending on the archive that is being >>>>> dealt >>>>> with. >>>>> >>>>> -Jaikiran >>>>> >>>>> >>>>> >>>>> On 17/05/18 12:04 PM, Maarten Coene wrote: >>>>> >>>>> Converting an Enumeration to a List just for iterating it doesn't seem >>>>> >>>>>> performance and memory wise a good idea to me. >>>>>> Maarten >>>>>> >>>>>> Van: "gin...@apache.org" <gin...@apache.org> >>>>>> Aan: notificati...@ant.apache.org >>>>>> Verzonden: woensdag 16 mei 19:13 2018 >>>>>> Onderwerp: [1/2] ant git commit: Deprecate CollectionUtils and >>>>>> Enumerations; reduce explicit use of Enumeration >>>>>> Repository: ant >>>>>> Updated Branches: >>>>>> refs/heads/master ac35c0014 -> 070c3bc86 >>>>>> >>>>>> >>>>>> http://git-wip-us.apache.org/repos/asf/ant/blob/070c3bc8/src >>>>>> /main/org/apache/tools/ant/types/ZipScanner.java >>>>>> ------------------------------------------------------------ >>>>>> ---------- >>>>>> diff --git a/src/main/org/apache/tools/ant/types/ZipScanner.java >>>>>> b/src/main/org/apache/tools/ant/types/ZipScanner.java >>>>>> index a3df040..5667159 100644 >>>>>> --- a/src/main/org/apache/tools/ant/types/ZipScanner.java >>>>>> +++ b/src/main/org/apache/tools/ant/types/ZipScanner.java >>>>>> @@ -20,7 +20,7 @@ package org.apache.tools.ant.types; >>>>>> import java.io.File; >>>>>> import java.io.IOException; >>>>>> -import java.util.Enumeration; >>>>>> +import java.util.Collections; >>>>>> import java.util.Map; >>>>>> import java.util.zip.ZipException; >>>>>> @@ -62,10 +62,7 @@ public class ZipScanner extends ArchiveScanner >>>>>> { >>>>>> "Only file provider resources are supported")); >>>>>> try (ZipFile zf = new ZipFile(srcFile, encoding)) { >>>>>> - >>>>>> - Enumeration<ZipEntry> e = zf.getEntries(); >>>>>> - while (e.hasMoreElements()) { >>>>>> - ZipEntry entry = e.nextElement(); >>>>>> + for (ZipEntry entry : Collections.list(zf.getEntries())) >>>>>> { >>>>>> Resource r = new ZipResource(srcFile, encoding, >>>>>> entry); >>>>>> String name = entry.getName(); >>>>>> if (entry.isDirectory()) { >>>>>> >>>>>> >>>>>> >>>>>> >>>>>> >>>>>> --------------------------------------------------------------------- >>>>> To unsubscribe, e-mail: dev-unsubscr...@ant.apache.org >>>>> For additional commands, e-mail: dev-h...@ant.apache.org >>>>> >>>>> >>>>> >>>>> --------------------------------------------------------------------- >>> To unsubscribe, e-mail: dev-unsubscr...@ant.apache.org >>> For additional commands, e-mail: dev-h...@ant.apache.org >>> >>> >>> > > --------------------------------------------------------------------- > To unsubscribe, e-mail: dev-unsubscr...@ant.apache.org > For additional commands, e-mail: dev-h...@ant.apache.org > >
