Community
Participate
Working Groups
Platform text plugins, once checked out from Git, are always not compilable because they insist to have Java 1.4 compatible JRE. There is no such JRE for download anymore, so even if I would accept this requirement I couldn't actually have it on my workstation. So while the platform UI is moved to Java 7 in 4.5, and parts of the platform are moving to Java 8 in 4.6, let us to relax the JRE criteria and move platform text to Java 7 *at least*. If it would be OK, we could just go to Java 8, but it is up to the team to decide. But Java 7 is a must have. P.S. I offer to perform some cleanup too after move (generic warnings, override annotation, unneeded casts etc), so this should not be a show stopper. The patch I propose first is a simple version increment in manifests, poms etc plus relaxing compiler setting to avoid too many new warnings. I've prepared a patch.
For whatever reason gerrit trigger didn't added the link to the patch, but here is it: https://git.eclipse.org/r/57012
New Gerrit change created: https://git.eclipse.org/r/57012
New Gerrit change created: https://git.eclipse.org/r/57014 WARNING: this patchset contains 4977 new lines of code and may require a Contribution Questionnaire (CQ) if the author is not a committer on the project. Please see:https://wiki.eclipse.org/Project_Management_Infrastructure/Creating_A_Contribution_Questionnaire
Created attachment 257171 [details] Big patches not accepted by Gerrit After patch https://git.eclipse.org/r/57014 I've cleaned up the code but Gerrit refuses to accept them (too big?), so I've exported them via CLI Git.
New Gerrit change created: https://git.eclipse.org/r/57852
New Gerrit change created: https://git.eclipse.org/r/57851
New Gerrit change created: https://git.eclipse.org/r/57850
New Gerrit change created: https://git.eclipse.org/r/57858
New Gerrit change created: https://git.eclipse.org/r/57857
New Gerrit change created: https://git.eclipse.org/r/57856
New Gerrit change created: https://git.eclipse.org/r/57855
New Gerrit change created: https://git.eclipse.org/r/57854
New Gerrit change created: https://git.eclipse.org/r/57853
New Gerrit change created: https://git.eclipse.org/r/57865
New Gerrit change created: https://git.eclipse.org/r/57864
New Gerrit change created: https://git.eclipse.org/r/57863
New Gerrit change created: https://git.eclipse.org/r/57861
New Gerrit change created: https://git.eclipse.org/r/57862
New Gerrit change created: https://git.eclipse.org/r/57860
New Gerrit change created: https://git.eclipse.org/r/57859
New Gerrit change created: https://git.eclipse.org/r/57871
New Gerrit change created: https://git.eclipse.org/r/57870
New Gerrit change created: https://git.eclipse.org/r/57869
New Gerrit change created: https://git.eclipse.org/r/57868
New Gerrit change created: https://git.eclipse.org/r/57867
New Gerrit change created: https://git.eclipse.org/r/57866
(In reply to Andrey Loskutov from comment #4) > Created attachment 257171 [details] > Big patches not accepted by Gerrit > > After patch https://git.eclipse.org/r/57014 I've cleaned up the code but > Gerrit refuses to accept them (too big?), so I've exported them via CLI Git. I've split all commits to smaller chunks, attachment can be removed but I have no idea how.
(In reply to Andrey Loskutov from comment #27) > (In reply to Andrey Loskutov from comment #4) > > Created attachment 257171 [details] > > Big patches not accepted by Gerrit > > > > After patch https://git.eclipse.org/r/57014 I've cleaned up the code but > > Gerrit refuses to accept them (too big?), Did you file a bug report? > I've split all commits to smaller chunks, attachment can be removed but I > have no idea how. I don't want n commits to solve one bug. You can attach a single patch or try with Gerrit again. Question: Are the changes simply based on running Infer Type Arguments...?
(In reply to Dani Megert from comment #28) > (In reply to Andrey Loskutov from comment #27) > > (In reply to Andrey Loskutov from comment #4) > > > Created attachment 257171 [details] > > > Big patches not accepted by Gerrit > > > > > > After patch https://git.eclipse.org/r/57014 I've cleaned up the code but > > > Gerrit refuses to accept them (too big?), > > Did you file a bug report? I assumed that this is just some limit (the patches *are* big), so I did not. > > > I've split all commits to smaller chunks, attachment can be removed but I > > have no idea how. > > I don't want n commits to solve one bug. Why not? They are so easier to review. I spent considerable amount of time to separate changes to manageable chunks:( > You can attach a single patch or > try with Gerrit again. > > Question: Are the changes simply based on running Infer Type Arguments...? First round yes, but it didn't worked well automatically in few cases and so I manually fixed errors where needed.
The committers team can decide if they prefer a single big commit or multiple small gerrit patches. If it helps that I take time to squash all the Gerrit changes in one single commit, so that we can start the IP review for a single big commit, I can invest some time to make it happens. I need Java 7 for the solution I proposed for Bug 75981.
(In reply to Jeremie Bresson from comment #30) > The committers team can decide if they prefer a single big commit or > multiple small gerrit patches. > > If it helps that I take time to squash all the Gerrit changes in one single > commit, so that we can start the IP review for a single big commit, I can > invest some time to make it happens. > > I need Java 7 for the solution I proposed for Bug 75981. Jeremy, squashing is a problem because of the mandatory IP check (and another long delay) if the contribution is over 1000 lines. The main problem here is to get responsible committers time to review the change(s) :-) @Markus, Dani mentioned that you are going to review the change soon and may be even we could switch to Java 8 directly. Can I help you somehow? Should I update the patches to Java 8 and I re-title the changes to something more meaningful as "Move platform text to Java 1.7 BREE"?
(In reply to Andrey Loskutov from comment #31) > @Markus, Dani mentioned that you are going to review the change soon and may > be even we could switch to Java 8 directly. Markus is working on it as we speak. Stay tuned!
I've started the review and will move all projects to 1.8 today or tomorrow. No new patches required. I'll not release the whitespace changes, since they only add noise to the history and are not a problem in practice. The review of the generification will take some time because it also affects APIs. When I checked out the last Gerrit change, I got compile errors in JDT/UI. Again, no new patches for that are required (they would just slow me down).
@Markus, you can instruct Git and EGit to ignore whitespace changes in the history.
(In reply to Lars Vogel from comment #34) > @Markus, you can instruct Git and EGit to ignore whitespace changes in the > history. No, I can only enable "git blame -w" in Show Annotations. This doesn't remove the commits from the history. And since this option also skips relevant whitespace changes (e.g. inside string literals or in properties files), I cannot enable it. Bug 483109 is a request to implement git-replace in JGit. This would improve the situation.
Sorry, I couldn't use the Gerrits. There were too many loose ends, e.g.: - too many warnings were just set to Ignore and then ignored - unnessary jre.compilation.profile in build.properties (missing in org.eclipse.ui.editors; doesn't increase my confidence) - not all bundle versions were increased (org.eclipse.text) - the whitespace changes have nothing to do with this bug and definitely can't be intermingled with other large-scale commits - I checked the first step of https://wiki.eclipse.org/Generify_A_Java_Project#Generify_your_project . - org.eclipse.text.edits.TextEdit.InsertionComparator was still raw. - org.eclipse.jface.text.link.TabStopIterator: If you start with generifying SequenceComparator, you see immediately that the right parameterization is <LinkedPosition>, and not <Position>. The #addPosition(Position) method had a bad argument type, but that can easily be fixed. - org.eclipse.ui.texteditor.rulers.RulerColumnRegistry#getColumnDescriptors() already specifies the element type, so <Object> is clearly wrong. - DAG and Multimap are collection classes and need type parameters - MetaIterator(Iterator<Iterator> iterator) is an atrocity I don't have time and motivation to review or iterate over such incomplete and API-breaking patches. Fixed with http://git.eclipse.org/c/platform/eclipse.platform.text.git/commit/?id=96da92a20589d62dad534245c5d9c4d45618ee95 and 3 parents: 0bbe21e0e553957dd09eda0b29e8c77a98f38048 64b34b614087b96e6cc92c72143768b98ae32602 f01dd079500e382823d5d1767996cb9ef85233ee The compile errors in JDT and other projects were due to APIs like org.eclipse.jface.text.source.projection.ProjectionAnnotationModel#modifyAnnotations(Annotation[], Map<? extends Annotation, ? extends Position>, Annotation[]), where the "? extends" are crucial. All projects now compile without any use of raw types and and with very few unavoidable @SuppressWarnings("unchecked"). A few downstream projects will see unused cast problem from the generified getAdapter(..). Remaining problems
*** Bug 462604 has been marked as a duplicate of this bug. ***
*** Bug 479526 has been marked as a duplicate of this bug. ***
96da92a20589d62dad534245c5d9c4d45618ee95 has caused a build failure for Mylyn because the return type of org.eclipse.jface.text.source.IAnnotationModel.getAnnotationIterator() was changed from Iterator to Iterator<Annotation>, whereas we implement that method to return Iterator<CommentAnnotation>. Does this change count as breaking API? Could IAnnotationModel.getAnnotationIterator() be changed to return Iterator<? extends Annotation>?
(In reply to Sam Davis from comment #39) > 96da92a20589d62dad534245c5d9c4d45618ee95 has caused a build failure for > Mylyn because the return type of > org.eclipse.jface.text.source.IAnnotationModel.getAnnotationIterator() was > changed from Iterator to Iterator<Annotation>, whereas we implement that > method to return Iterator<CommentAnnotation>. Does this change count as > breaking API? Could IAnnotationModel.getAnnotationIterator() be changed to > return Iterator<? extends Annotation>? Similar problem with many types in org.eclipse.jface.text.source which return <Annotation> and <Position> generified variants of collections or iterators like IAnnotationMap etc. I think because we've introduced generics in 4.6 M4 we still can change the interfaces during 4.6 development, but not later. So the question is if we should change generified interfaces accordingly to allow subclasses of Annotation and Position. I've prepared a change set, it would be nice if you could review it.
New Gerrit change created: https://git.eclipse.org/r/62765
Done. Thanks for jumping on this so quickly! Note that there was nothing stopping clients from implementing that method to return Iterator<String> for example, even though that clearly makes no sense. I'm not sure if it matters that such clients would fail to compile even with the proposed fix, or if there are other cases where returning a type like that could make sense.
(In reply to Sam Davis from comment #42) > Done. Thanks for jumping on this so quickly! Note that there was nothing > stopping clients from implementing that method to return Iterator<String> > for example, even though that clearly makes no sense. I would say it was expected that the collections do not contain unrelated types. > I'm not sure if it > matters that such clients would fail to compile even with the proposed fix, > or if there are other cases where returning a type like that could make > sense. What should be checked if we have similar cases where generified API uses collections or iterators of something which can be subclassed. With the search query below I see 39 matches in non internal classes in platform text: ((public)|(protected))\s+.*<\s*\p{Upper}\w+\s*> Question is: how many from this API's need to be fixed too? There are for example similar cases with iterators: AnnotationBag.java 81: public Iterator<Annotation> iterator() CompositeRuler.java 760: public Iterator<IVerticalRulerColumn> getDecoratorIterator() ContextTypeRegistry.java 57: public Iterator<TemplateContextType> contextTypes() DocumentCommand.java 338: public Iterator<Command> getCommandIterator() TextPresentation.java 608: public Iterator<StyleRange> getNonDefaultStyleRangeIterator() 619: public Iterator<StyleRange> getAllStyleRangeIterator() TemplateContextType.java 177: public Iterator<TemplateVariableResolver> resolvers() TextFileDocumentProvider.java 1.253: protected Iterator<FileInfo> getFileInfosIterator()
*** Bug 475590 has been marked as a duplicate of this bug. ***
One reason why I left e.g. IAnnotationModel#getAnnotationIterator() to return an Iterator<Annotation> was that the Javadoc used to say: * @return all annotations managed by this model (element type: {@link Annotation}) In the SDK, we already had callers that assumed this means Iterator<Annotation> and that would get a compile error if the API returns Iterator<? extends Annotation>. Example in org.eclipse.ant.internal.ui.editor.text.AntFoldingStructureProvider#computeDifferences(ProjectionAnnotationModel, Set<Position>): for (Iterator<Annotation> iter = model.getAnnotationIterator(); iter.hasNext();) { Thanks for the Gerrit, Andrey. I'll review and see if it's easy to fix the other clients in the SDK. We will probably release this change, but only after it's clear that we don't run into trouble with other APIs, and only after SDK clients have been updated.
(In reply to Sam Davis from comment #39) > [..] the return type of > org.eclipse.jface.text.source.IAnnotationModel.getAnnotationIterator() was > changed from Iterator to Iterator<Annotation>, whereas we implement that > method to return Iterator<CommentAnnotation>. Does this change count as > breaking API? No, it's binary compatible and the source breakage should be fixable on the client side. > Could IAnnotationModel.getAnnotationIterator() be changed to > return Iterator<? extends Annotation>? It could, but I'd prefer not to. It would already require tons of changes in the text plug-ins, about 20 more in the Eclipse SDK, and probably many more in third-party plug-ins. https://docs.oracle.com/javase/tutorial/java/generics/wildcardGuidelines.html agrees with that. (In reply to Andrey Loskutov from comment #43) > Question is: how many from this API's need to be fixed too? There are for > example similar cases with iterators: We should only change methods that can be overridden by clients, and where we expect clients to actually need/want to change the bound. For other methods that return an Iterator, adding a wildcard just makes it cumbersome for clients to work with the returned value. In the JRE libraries, there are very few methods that return a bounded wildcard type. One of the rare places where they actually make use of the flexible return type is this: java.util.zip.ZipFile#entries() : Enumeration<? extends ZipEntry> java.util.jar.JarFile#entries() : Enumeration<JarEntry> In many other cases, methods return a concrete parameterized type, e.g.: java.nio.charset.spi.CharsetProvider#charsets() : Iterator<Charset> java.nio.file.spi.FileSystemProvider#newDirectoryStream(Path, Filter<? super Path>) : DirectoryStream<Path> java.nio.file.FileSystem#getFileStores() : Iterable<FileStore> Here's a longer discussion on this topic: http://www.angelikalanger.com/GenericsFAQ/FAQSections/ProgrammingIdioms.html#FAQ303 => Changing the Iterator<Annotation> in AnnotationModel to Iterator<? extends Annotation> would be acceptable for me, although I don't think it's really necessary. I have the prerequisite changes in 4 SDK repos ready: eclipse.platform, eclipse.platform.debug, eclipse.jdt.debug, eclipse.jdt.ui The parameterization of IAnnotationMap is a step too much and is not necessary to fix Sam's build failure (comment 39). The AnnotationModel#fAnnotations field needs to keep the fixed type Map<Annotation, Position> anyway, since #put(..) is not possible if one of the type parameters is a "? extends" type. Changing AnnotationModel#getAnnotationMap() to return IAnnotationMap<? extends Annotation, ? extends Position> is a breaking change, since this removes the possibility for callers to put anything into the map. => Currently, there's no reason for making IAnnotationMap/AnnotationMap generic. Should a reason arise, then we can still parameterize it later. And if we started that, we could also be tempted to turn the type Annotation in AnnotationModel into a type parameter. But I don't see much benefit in doing so, and it would cause a lot of ripples. > New Gerrit change created: https://git.eclipse.org/r/62765 IAnnotationMap and AnnotationModel.RegionIterator would have required their new Javadoc warnings fixed. Unless there are any strong arguments in favor of doing the change, I'd close this bug again.
(In reply to comment #46) > (In reply to Sam Davis from comment #39) > > [..] the return type of > > org.eclipse.jface.text.source.IAnnotationModel.getAnnotationIterator() was > > changed from Iterator to Iterator<Annotation>, whereas we implement that > > method to return Iterator<CommentAnnotation>. Does this change count as > > breaking API? > > No, it's binary compatible and the source breakage should be fixable on the > client side. Makes sense, thanks for clarifying that. > > > Could IAnnotationModel.getAnnotationIterator() be changed to > > return Iterator<? extends Annotation>? > > It could, but I'd prefer not to. It would already require tons of changes in the > text plug-ins, about 20 more in the Eclipse SDK, and probably many more in > third-party plug-ins. > https://docs.oracle.com/javase/tutorial/java/generics/wildcardGuidelines.html > agrees with that. > That doesn't really talk about the problem I'm running into, which is that clients need to implement the method with a generic return type. Since the Annotation class is not final or marked with @NoExtend, it would be nice if the model supported clients who need to extend it. > > Unless there are any strong arguments in favor of doing the change, I'd close > this bug again. I think we can fix this fairly easily in Mylyn, so please do what you think is best and we'll deal with it.
We'll keep the APIs as released in 4.6 M4.
*** Bug 431659 has been marked as a duplicate of this bug. ***
*** Bug 469236 has been marked as a duplicate of this bug. ***
