Lucene - Core
  1. Lucene - Core
  2. LUCENE-1855

Change AttributeSource API to use generics

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 3.0
    • Component/s: modules/analysis
    • Labels:
      None
    • Lucene Fields:
      New

      Description

      The AttributeSource API will be easier to use with JDK 1.5 generics.

      Uwe, if you started working on a patch for this already feel free to assign this to you.

      1. AttributeSource.jad
        13 kB
        Uwe Schindler
      2. LUCENE-1855_contrib_queryparser.patch
        45 kB
        Adriano Crestani
      3. LUCENE-1855_contrib.patch
        97 kB
        Robert Muir
      4. LUCENE-1855_precedence_queryparser.patch
        73 kB
        Adriano Crestani
      5. LUCENE-1855.patch
        77 kB
        Uwe Schindler
      6. LUCENE-1855.patch
        77 kB
        Uwe Schindler
      7. LUCENE-1855.patch
        71 kB
        Uwe Schindler
      8. LUCENE-1855.patch
        71 kB
        Uwe Schindler
      9. LUCENE-1855.patch
        22 kB
        Uwe Schindler
      10. LUCENE-1855.patch
        19 kB
        Uwe Schindler

        Issue Links

          Activity

          Hide
          Uwe Schindler added a comment -

          OK, will do. Most work is removing the casts from all TokenStream ctors
          Currently I just tried out with two classes.

          Show
          Uwe Schindler added a comment - OK, will do. Most work is removing the casts from all TokenStream ctors Currently I just tried out with two classes.
          Hide
          Uwe Schindler added a comment -

          Here is the patch that implements generics for the Attributes API. The test shows, how simple now usage of addAttribute() and so on is (no casts needed anymore).

          The code compiles without any unchecked warnings. To test this, I enabled the warnings about unchecked operations globally (emitting about 350 warnings on whole lucene). Two places inside private code needs to add @SuppressWarnings, because the compiler does not know if one AttributeImpl really implements the questioned interface.

          Show
          Uwe Schindler added a comment - Here is the patch that implements generics for the Attributes API. The test shows, how simple now usage of addAttribute() and so on is (no casts needed anymore). The code compiles without any unchecked warnings. To test this, I enabled the warnings about unchecked operations globally (emitting about 350 warnings on whole lucene). Two places inside private code needs to add @SuppressWarnings, because the compiler does not know if one AttributeImpl really implements the questioned interface.
          Hide
          Uwe Schindler added a comment -

          As I already started to move Lucene to Java 1.5, here an idea, how to get the generics in:

          I would propose to switch on all "unchecked" warnings (as in the supplied patch) by the <compilerarg/> in ant. The problem is then, that there are hundreds of warnings printed out. I would like to then add a @SuppressWarnings("unchecked") to all classes, that are not yet rewritten to generics for collections and other generified java things (like Class<?> in AttributeSource). The warnings should then disappear. We could then start to search for SuppressWarnings annotations in the source and start the classes one-by-one and add generics. By this it is simplier, because you only get warnings for the class you are working on.

          What do you think?

          Show
          Uwe Schindler added a comment - As I already started to move Lucene to Java 1.5, here an idea, how to get the generics in: I would propose to switch on all "unchecked" warnings (as in the supplied patch) by the <compilerarg/> in ant. The problem is then, that there are hundreds of warnings printed out. I would like to then add a @SuppressWarnings("unchecked") to all classes, that are not yet rewritten to generics for collections and other generified java things (like Class<?> in AttributeSource). The warnings should then disappear. We could then start to search for SuppressWarnings annotations in the source and start the classes one-by-one and add generics. By this it is simplier, because you only get warnings for the class you are working on. What do you think?
          Hide
          Uwe Schindler added a comment - - edited

          About the backwards compatibility: After adding generics, the backwards test should run all tests from 2.9 (compiled with 1.4) against the generified trunk jar. After branching I would start to manage this.

          Normally generics do not bring backwards incompatibility, because they are simple removed. You only have probloems at places, where the the erased generics should not be replaced by java.lang.Object. Eg in the AttributeSource.addAttribute() call, it should return a Attribute (subclass) as in 2.9 and not Object (because of this you need to generify the whole method by the generics prefix, defining "A" as subclass of "Attribute"). If you use an naive approach to add generics, it could lead to addAttribute returns Object (and so a link error would occur). To prevent this, the backwards tests against 2.9 are a good solution.

          By the way, the generics for AttributeSource were copied from j.l.Class and its methods to get annotations

          Show
          Uwe Schindler added a comment - - edited About the backwards compatibility: After adding generics, the backwards test should run all tests from 2.9 (compiled with 1.4) against the generified trunk jar. After branching I would start to manage this. Normally generics do not bring backwards incompatibility, because they are simple removed. You only have probloems at places, where the the erased generics should not be replaced by java.lang.Object. Eg in the AttributeSource.addAttribute() call, it should return a Attribute (subclass) as in 2.9 and not Object (because of this you need to generify the whole method by the generics prefix, defining "A" as subclass of "Attribute"). If you use an naive approach to add generics, it could lead to addAttribute returns Object (and so a link error would occur). To prevent this, the backwards tests against 2.9 are a good solution. By the way, the generics for AttributeSource were copied from j.l.Class and its methods to get annotations
          Hide
          Mark Miller added a comment -

          By the way, the generics for AttributeSource were copied from j.l.Class and its methods to get annotations

          From the apache harmony project I hope

          Show
          Mark Miller added a comment - By the way, the generics for AttributeSource were copied from j.l.Class and its methods to get annotations From the apache harmony project I hope
          Hide
          Michael Busch added a comment -

          From the apache harmony project I hope

          No worries... the original patch (LUCENE-1422) had the 1.5 version attached as comments all the time and wasn't copied from anywhere. I originally designed the API with 1.5 generics and ported it back to 1.4 to be able to commit it.

          We just removed those comments with some patch from trunk (probably LUCENE-1693) and Uwe brought it back now.

          Show
          Michael Busch added a comment - From the apache harmony project I hope No worries... the original patch ( LUCENE-1422 ) had the 1.5 version attached as comments all the time and wasn't copied from anywhere. I originally designed the API with 1.5 generics and ported it back to 1.4 to be able to commit it. We just removed those comments with some patch from trunk (probably LUCENE-1693 ) and Uwe brought it back now.
          Hide
          Uwe Schindler added a comment -

          From the apache harmony project I hope

          No problem at all, it was more only this public API line that inspired me... I did not copy any code, only the public API:

          public <A extends Annotation> A getAnnotation(Class<A> annotationClass)
          

          from http://java.sun.com/j2se/1.5.0/docs/api/java/lang/Class.html#getAnnotation(java.lang.Class).

          Michael: This was not in the comments (at least not in the comments from the latest Lucene code before our rewrite).

          Show
          Uwe Schindler added a comment - From the apache harmony project I hope No problem at all, it was more only this public API line that inspired me... I did not copy any code, only the public API: public <A extends Annotation> A getAnnotation( Class <A> annotationClass) from http://java.sun.com/j2se/1.5.0/docs/api/java/lang/Class.html#getAnnotation(java.lang.Class) . Michael: This was not in the comments (at least not in the comments from the latest Lucene code before our rewrite).
          Hide
          Mark Miller added a comment -

          no worries here either guys - far from the copyright police here - just a wink comment.

          Show
          Mark Miller added a comment - no worries here either guys - far from the copyright police here - just a wink comment.
          Hide
          Michael Busch added a comment -

          Michael: This was not in the comments (at least not in the comments from the latest Lucene code before our rewrite).

          It was. The LUCENE-1693 commit removed it. And my first 1693 patch left it in, so either me or you removed it in the subsequent 1693 patches.
          It's not really important I guess As you and Mark said: it's just public API stuff that we could have gotten from the javadocs anyway.

          Show
          Michael Busch added a comment - Michael: This was not in the comments (at least not in the comments from the latest Lucene code before our rewrite). It was. The LUCENE-1693 commit removed it. And my first 1693 patch left it in, so either me or you removed it in the subsequent 1693 patches. It's not really important I guess As you and Mark said: it's just public API stuff that we could have gotten from the javadocs anyway.
          Hide
          Uwe Schindler added a comment -

          It was.

          OK. I found it - in the first version after checkin.

          Attached is a new patch, now also making TokenStream generics activated. The next step is to convert all Tokenizers (as always...). Michael: Is this patch, how you want to have it?

          Show
          Uwe Schindler added a comment - It was. OK. I found it - in the first version after checkin. Attached is a new patch, now also making TokenStream generics activated. The next step is to convert all Tokenizers (as always...). Michael: Is this patch, how you want to have it?
          Hide
          Michael Busch added a comment -

          Uwe, I'm not home right now, will look tonight! Thanks for writing the patch!

          Show
          Michael Busch added a comment - Uwe, I'm not home right now, will look tonight! Thanks for writing the patch!
          Hide
          Uwe Schindler added a comment -

          For all developers that want to see, that the generics-enabled class AttributeSource is binary compatible (so addAttribute still return Attribute in the erasured class) to the 2.9 version, here is the de-compiled version of the class file (using JAD). This is not really full featured java code, but shows, how the erasured class looks like (and how javac generates the advanced FOR statements) and the covariant overriding of iterator's next(). Really interesting.

          JAD is always good, to look, how a class with generic looks like for code compiled agains 1.4.

          Show
          Uwe Schindler added a comment - For all developers that want to see, that the generics-enabled class AttributeSource is binary compatible (so addAttribute still return Attribute in the erasured class) to the 2.9 version, here is the de-compiled version of the class file (using JAD). This is not really full featured java code, but shows, how the erasured class looks like (and how javac generates the advanced FOR statements) and the covariant overriding of iterator's next(). Really interesting. JAD is always good, to look, how a class with generic looks like for code compiled agains 1.4.
          Hide
          Michael Busch added a comment -

          Patch looks great, Uwe!

          Show
          Michael Busch added a comment - Patch looks great, Uwe!
          Hide
          Uwe Schindler added a comment -

          As we are now on Java 1.5, I would like to go forward with this issue as the first real patch using generics.

          During development of generics, it is really important that you see all unchecked warnings, so this patch also adds <compilerarg line="-Xlint:unchecked"/> to the javac task. The problem is now, that all non-generics classes in Lucene generate tons of warnings. I would like to proceed like the following:

          • Open a new issue that adds this compilerarg.
          • Add a @SuppressWarnings("unchecked") in front of each class, that (currently) produces warnings (should not be so many, hopefully).

          After that the compilation should run without warnings about unchecked casts and so on. When starting to add generics to some of the classes, the developer would remove the SuppressWarnings for the class he works on and start fixing the warnngs and adding generics.

          What do you think? Without that, it is hard to add the generics 100% safe and consistent without be lost in tons of warnings and so on.

          I will also update this patch to also remove the now unneeded casts in all addAttribute calls in Lucene (low priority).

          Show
          Uwe Schindler added a comment - As we are now on Java 1.5, I would like to go forward with this issue as the first real patch using generics. During development of generics, it is really important that you see all unchecked warnings, so this patch also adds <compilerarg line="-Xlint:unchecked"/> to the javac task. The problem is now, that all non-generics classes in Lucene generate tons of warnings. I would like to proceed like the following: Open a new issue that adds this compilerarg. Add a @SuppressWarnings("unchecked") in front of each class, that (currently) produces warnings (should not be so many, hopefully). After that the compilation should run without warnings about unchecked casts and so on. When starting to add generics to some of the classes, the developer would remove the SuppressWarnings for the class he works on and start fixing the warnngs and adding generics. What do you think? Without that, it is hard to add the generics 100% safe and consistent without be lost in tons of warnings and so on. I will also update this patch to also remove the now unneeded casts in all addAttribute calls in Lucene (low priority).
          Hide
          Uwe Schindler added a comment -

          Here the master patch prepared in the train from Berlin to Bremen. It converts everything TokenStream related to generics.

          To be sure, that non-generics 2.9 tests are binary compatible, I will first create a new backwards branch and modify build.xml to compile the backwards test withs -source and -target 1.4

          I will commit soon and go forward with NumericRange generification. The question about the SuppressWarnings-TODO is still open!

          Uwe

          Show
          Uwe Schindler added a comment - Here the master patch prepared in the train from Berlin to Bremen. It converts everything TokenStream related to generics. To be sure, that non-generics 2.9 tests are binary compatible, I will first create a new backwards branch and modify build.xml to compile the backwards test withs -source and -target 1.4 I will commit soon and go forward with NumericRange generification. The question about the SuppressWarnings-TODO is still open! Uwe
          Hide
          Uwe Schindler added a comment -

          Sorry, last patch had an error in hasReusableNext backwards layer initialization.

          Show
          Uwe Schindler added a comment - Sorry, last patch had an error in hasReusableNext backwards layer initialization.
          Hide
          Uwe Schindler added a comment -

          Some Javadoc additions, also change QueryParser.jj file.

          Contrib and contrib queryparser was not converted. The new Queryparser would also get cleaner without the unneeded casts.

          I will commit this tomorrow, when JIRA hopefully works better.

          Robert Muir: Do you have time to remove the casts from contrib?
          Luis Alves: You can also update the new Queryparser to not cas the return of getAttribute/addAttribute

          Show
          Uwe Schindler added a comment - Some Javadoc additions, also change QueryParser.jj file. Contrib and contrib queryparser was not converted. The new Queryparser would also get cleaner without the unneeded casts. I will commit this tomorrow, when JIRA hopefully works better. Robert Muir: Do you have time to remove the casts from contrib? Luis Alves: You can also update the new Queryparser to not cas the return of getAttribute/addAttribute
          Hide
          Robert Muir added a comment -

          patch for contrib (except for the new queryparser)

          Show
          Robert Muir added a comment - patch for contrib (except for the new queryparser)
          Hide
          Adriano Crestani added a comment -

          Here is the patch for contrib/queryparser

          Show
          Adriano Crestani added a comment - Here is the patch for contrib/queryparser
          Hide
          Uwe Schindler added a comment -

          A small update to my core patch, it now needs no @SuppressWarnings anymore in addAttribute/getAttribute impl

          I will commit soon.

          Thanks Robert for the contrib patch. I will do the new QueryParser later. The PrecedenceParser must be fixed or removed, when the old TokenStream API gets removed.

          Show
          Uwe Schindler added a comment - A small update to my core patch, it now needs no @SuppressWarnings anymore in addAttribute/getAttribute impl I will commit soon. Thanks Robert for the contrib patch. I will do the new QueryParser later. The PrecedenceParser must be fixed or removed, when the old TokenStream API gets removed.
          Hide
          Uwe Schindler added a comment -

          Oh thanks Adriano, I haven't seen your patch! I will add it!

          Thank you very much!

          Show
          Uwe Schindler added a comment - Oh thanks Adriano, I haven't seen your patch! I will add it! Thank you very much!
          Hide
          Adriano Crestani added a comment -

          This patch updates the PQP to use the new TokenStream API...all tests still pass.

          I hope this helps to keep the PQP

          Show
          Adriano Crestani added a comment - This patch updates the PQP to use the new TokenStream API...all tests still pass. I hope this helps to keep the PQP
          Hide
          Uwe Schindler added a comment -

          Thanks Adriano, I will open another issue about that, as it has not really something to do with this issue!

          Show
          Uwe Schindler added a comment - Thanks Adriano, I will open another issue about that, as it has not really something to do with this issue!
          Hide
          Uwe Schindler added a comment -

          Added CHANGES.txt and committed revision: 820553

          Show
          Uwe Schindler added a comment - Added CHANGES.txt and committed revision: 820553

            People

            • Assignee:
              Uwe Schindler
              Reporter:
              Michael Busch
            • Votes:
              0 Vote for this issue
              Watchers:
              1 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development