Lucene.Net
  1. Lucene.Net
  2. LUCENENET-515

bring back TokenStream.GetAttribute(Type).

    Details

    • Type: Improvement Improvement
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: Lucene.Net 3.0.3
    • Fix Version/s: Lucene.Net 3.6
    • Component/s: Lucene.Net Core
    • Labels:
      None

      Description

      I Have noticed that TokenStream.GetAttribute(Type) is gone in favor for TokenStream.GetAttribute<Type>();

      Obviously TokenStream.GetAttribute<Type>() is a better syntax; but it should not replace TokenStream.GetAttribute(Type), but instead compliment it... But this is a common pitfall, especially for non-.NET developers and people who have not run into the implications of this..

      In the case of Lucene.NET, it is properly unlikely that this will affect anyone, however, I consider what has been done bad practice... now why is that?

      Many would think like this:

      Before I had to do this old thing...

      TokenStream.GetAttribute(typeof(TermAttribute));

      And god I hated that, so now I replaced it with the much more beautiful:

      TokenStream.GetAttribute<TermAttribute>();

      And deleted that old hag of a method taking a Type... And now I am happy...

      BUT!...

      What when...

      Type myNotDefinedHereType = GetTypeFromSomeWhere();
      TokenSteam.GetAttribute.... ?????!?!?!?... Uhm... What now???? 
      

      Now you have to write a whole lot of reflection mess, use a dynamically compiled delegate using IL-Emit or the Mono Compiler as a Service...
      All of those 3 workarounds are generally just ugly...

      If you keept both...

      Type myNotDefinedHereType = GetTypeFromSomeWhere();
      TokenSteam.GetAttribute(myNotDefinedHereType);
      

      While it might be unlikely that it will ever be used, there is always the off case, and API's should support both...

      So instead of:

      public virtual T GetAttribute<T>() where T : IAttribute
          {
            Type key = typeof (T);
            if (!this.attributes.ContainsKey(key))
              throw new ArgumentException("This AttributeSource does not have the attribute '" + key.FullName + "'.");
            else
              return (T) this.attributes[key].Value;
          }
      

      Do:

      public T GetAttribute<T>() where T : IAttribute
      {
         return (T) GetAttribute(typeof(T));
      } 
      public virtual IAttribute GetAttribute(Type key)
          {
            //Note: Since Type is required to implement IAttribute, I would properly check that as well to be able to provide a more meaningfull error... However to speed things up, I would do it inside the if bellow...
            if (!this.attributes.ContainsKey(key))
              throw new ArgumentException("This AttributeSource does not have the attribute '" + key.FullName + "'.");
            else
              return (T) this.attributes[key].Value;
          }
      

        Activity

        Hide
        Simon Svensson added a comment - - edited

        IAttribute is just an empty marker interface, wouldn't you still need "a whole lot of reflection mess" to interact with it if you had an IAttribute GetAttribute(Type) api?

        Show
        Simon Svensson added a comment - - edited IAttribute is just an empty marker interface, wouldn't you still need "a whole lot of reflection mess" to interact with it if you had an IAttribute GetAttribute(Type) api?
        Hide
        Jens Melgaard added a comment -

        Weather you would return IAttribute or just object doesn't matter, however it would makes sense to return IAttribute since it seems you require that interface...
        But actually, no... today you wouldn't need a whole lot of reflection to interact with an object of unknown type, you could let the .NET framework handle that part for you since 4.0 (dynamics)...
        I don't know the nature of the attributes we talk about here... So I wouldn't know what kind of interactions that would make sense...
        You might not even be the one interacting with it...

        I am merely pointing out what I see as bad practice from an API perspective. Because I have meet these challenges before where we actually overcame them by changing framework...

        Show
        Jens Melgaard added a comment - Weather you would return IAttribute or just object doesn't matter, however it would makes sense to return IAttribute since it seems you require that interface... But actually, no... today you wouldn't need a whole lot of reflection to interact with an object of unknown type, you could let the .NET framework handle that part for you since 4.0 (dynamics)... I don't know the nature of the attributes we talk about here... So I wouldn't know what kind of interactions that would make sense... You might not even be the one interacting with it... I am merely pointing out what I see as bad practice from an API perspective. Because I have meet these challenges before where we actually overcame them by changing framework...
        Hide
        Michael Herndon added a comment -

        TokenStream.GetAttribute(Type).
        MIME-Version: 1.0
        Content-Type: text/plain; charset="utf-8"
        Content-Transfer-Encoding: 7bit

        You would, but it generally is a good idea to support both styles in
        case the type is dynamically deduced for whatever reason.

        Sent from my Windows Phone
        From: Simon Svensson (JIRA)
        Sent: 12/7/2012 7:55 AM
        To: dev@lucenenet.apache.org
        Subject: [jira] [Commented] (LUCENENET-515) bring back
        TokenStream.GetAttribute(Type).

        [ https://issues.apache.org/jira/browse/LUCENENET-515?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13526351#comment-13526351
        ]

        Simon Svensson commented on LUCENENET-515:
        ------------------------------------------

        IAttribute is just an empty marker interface, wouldn't you still need
        "a whole lot of reflection mess" to interact with it if you had an
        `IAttribute GetAttribute(Type)` api?


        This message is automatically generated by JIRA.
        If you think it was sent incorrectly, please contact your JIRA administrators
        For more information on JIRA, see: http://www.atlassian.com/software/jira

        Show
        Michael Herndon added a comment - TokenStream.GetAttribute(Type). MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit You would, but it generally is a good idea to support both styles in case the type is dynamically deduced for whatever reason. Sent from my Windows Phone From: Simon Svensson (JIRA) Sent: 12/7/2012 7:55 AM To: dev@lucenenet.apache.org Subject: [jira] [Commented] ( LUCENENET-515 ) bring back TokenStream.GetAttribute(Type). [ https://issues.apache.org/jira/browse/LUCENENET-515?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13526351#comment-13526351 ] Simon Svensson commented on LUCENENET-515 : ------------------------------------------ IAttribute is just an empty marker interface, wouldn't you still need "a whole lot of reflection mess" to interact with it if you had an `IAttribute GetAttribute(Type)` api? – This message is automatically generated by JIRA. If you think it was sent incorrectly, please contact your JIRA administrators For more information on JIRA, see: http://www.atlassian.com/software/jira
        Hide
        Christopher Currens added a comment -

        Two lines of reflection code isn't what I would consider a "whole lot of reflection mess" to get what you want done.

            public static class AttributeSourceExtensions
            {
                public static IAttribute GetAttribute(this AttributeSource attributeSource, Type t)
                {
                    if(!typeof(IAttribute).IsAssignableFrom(t))
                        throw new ArgumentException("Type of T must inherit from IAttribute");
        
                    var mi = typeof(AttributeSource).GetMethod("GetAttribute").MakeGenericMethod(t);
                    return (IAttribute)mi.Invoke(attributeSource, null);
                }
            }
        

        You could easily do the same for HasAttribute. Here's an example:

                private static void Main(string[] args)
                {
                    string str = "This is my text";
                    var a = new StandardAnalyzer(Version.LUCENE_30);
        
                    var ts = a.TokenStream("Field", new StringReader(str));
        
                    ts.IncrementToken();
        
                    Console.WriteLine(ts.GetAttribute<ITermAttribute>().Term);
                    Console.WriteLine(((ITermAttribute) ts.GetAttribute(typeof(ITermAttribute))).Term);
                }
        

        I'm not saying that the API shouldn't have an overload that accepts a type, in fact I agree that it should. HOWEVER, Lucene 3.0.3 doesn't have a method like this in java, and so that is the reason why it was removed. Keep in mind that things run slowly around here, and so I'm not sure if or when this additional overload will be added for 3.0.3.

        Show
        Christopher Currens added a comment - Two lines of reflection code isn't what I would consider a "whole lot of reflection mess" to get what you want done. public static class AttributeSourceExtensions { public static IAttribute GetAttribute( this AttributeSource attributeSource, Type t) { if (!typeof(IAttribute).IsAssignableFrom(t)) throw new ArgumentException( "Type of T must inherit from IAttribute" ); var mi = typeof(AttributeSource).GetMethod( "GetAttribute" ).MakeGenericMethod(t); return (IAttribute)mi.Invoke(attributeSource, null ); } } You could easily do the same for HasAttribute . Here's an example: private static void Main(string[] args) { string str = "This is my text" ; var a = new StandardAnalyzer(Version.LUCENE_30); var ts = a.TokenStream( "Field" , new StringReader(str)); ts.IncrementToken(); Console.WriteLine(ts.GetAttribute<ITermAttribute>().Term); Console.WriteLine(((ITermAttribute) ts.GetAttribute(typeof(ITermAttribute))).Term); } I'm not saying that the API shouldn't have an overload that accepts a type, in fact I agree that it should. HOWEVER, Lucene 3.0.3 doesn't have a method like this in java, and so that is the reason why it was removed. Keep in mind that things run slowly around here, and so I'm not sure if or when this additional overload will be added for 3.0.3.
        Hide
        Jens Melgaard added a comment - - edited

        Christopher Currens

        you can compact many things into "one liners"... that doesn't make them less of a mess...
        The reason I say "A whole lot of" is A: Because of the steps required, B: Because it would be unnecessary and only ends up adding overhead...

        But to take the steps:

        1. First you have to get the type for IAttribute
        2. Then you have to verify that the type is assignable to that
          1. If not, then you need to break out (one way or another, throwing an exception makes sense)
        3. Then you have to get the type you wan't to call on.
        4. Then you have to get the method you wan't to call on that type
        5. Then you need to make a generic version of that method
        6. Then you need to call the method
        7. Then you need to return the value

        And yes, you did all in a very few lines of code, it doesn't change that you DO all that... as opposed to:

        1. Call the method
        2. Return the value

        I Realize however that this might be because the code is converted over form Java, one could question that ain't that same method missing in JAVA?... Now I haven't coded Java since just before 1.5 so I don't have a clue on what you would do in JAVA today, if there are other means of calling a method like this where you only know the exact type at runtime... So I won't question that...

        In any case, if we just "hoped" the team on Lucene.NET realized this by them self, it might never have happened, instead... why not report it as an improvement/defect or whatever so they can improve on their API... Isn't that in the end a help?...

        It is OK if it gets low priority to me...

        Show
        Jens Melgaard added a comment - - edited Christopher Currens you can compact many things into "one liners"... that doesn't make them less of a mess... The reason I say "A whole lot of" is A: Because of the steps required, B: Because it would be unnecessary and only ends up adding overhead... But to take the steps: First you have to get the type for IAttribute Then you have to verify that the type is assignable to that If not, then you need to break out (one way or another, throwing an exception makes sense) Then you have to get the type you wan't to call on. Then you have to get the method you wan't to call on that type Then you need to make a generic version of that method Then you need to call the method Then you need to return the value And yes, you did all in a very few lines of code, it doesn't change that you DO all that... as opposed to: Call the method Return the value I Realize however that this might be because the code is converted over form Java, one could question that ain't that same method missing in JAVA?... Now I haven't coded Java since just before 1.5 so I don't have a clue on what you would do in JAVA today, if there are other means of calling a method like this where you only know the exact type at runtime... So I won't question that... In any case, if we just "hoped" the team on Lucene.NET realized this by them self, it might never have happened, instead... why not report it as an improvement/defect or whatever so they can improve on their API... Isn't that in the end a help?... It is OK if it gets low priority to me...
        Hide
        Christopher Currens added a comment - - edited

        Jens, you've missed my point. We do need to add this, and I was just trying to be helpful by suggesting a simple solution, but instead I still am met with resistance from you, even though I agreed with you about adding an overload that accepted a type as a parameter. I suggested this simple workaround because I have my doubts that this would be added into a 3.0.3 maintenance release because it's such a low priority and we have little resources to spend on these things when we're focused on trying to port 3.6 and/or 4.0.

        And yes, you did all in a very few lines of code, it doesn't change that you DO all that... as opposed to:

        1. Call the method
        2. Return the value

        As opposed to just calling the method and returning the value? So where do you check if the type passed into the method is assignable from IAttribute? You're oversimplifying things here, because it still has to be done somewhere.

        However, there's no doubt that reflection would be slower than having a method dedicated to this with all of the proper checks and casting, but on the flip side, you can use Delegate.CreateDelegate and cache the result. Then, the only real additional cost is reflection required to initially create it. Subsequent runs would be as fast as a normal call (at least in this case. If the type's method body were shorter, it would have the opportunity to be inlined where the delegate would not).

        Anyway, this is slowly moving to a pointless argument about reflection, so, here's the bottom line:

        We will add an additional non-generic method overload for these generic ones. This could be done in a few short months, a year, or perhaps even longer. I would prefer months, but sometimes it just can't happen. In the meantime if you decide that you can't live without this, you can easily write code that uses reflection to get this done (it took me all of 10 minutes to write the above code). Any reflection code you write can be viewed as temporary, since we are going to add this additional non-generic method overload (I feel like I can't stress this enough). If you implement the reflection correctly, you will likely see no real measurable gain in performance when you are able to switch to using that non-generic method overload in the future.

        Show
        Christopher Currens added a comment - - edited Jens, you've missed my point. We do need to add this, and I was just trying to be helpful by suggesting a simple solution, but instead I still am met with resistance from you, even though I agreed with you about adding an overload that accepted a type as a parameter. I suggested this simple workaround because I have my doubts that this would be added into a 3.0.3 maintenance release because it's such a low priority and we have little resources to spend on these things when we're focused on trying to port 3.6 and/or 4.0. And yes, you did all in a very few lines of code, it doesn't change that you DO all that... as opposed to: Call the method Return the value As opposed to just calling the method and returning the value? So where do you check if the type passed into the method is assignable from IAttribute ? You're oversimplifying things here, because it still has to be done somewhere. However, there's no doubt that reflection would be slower than having a method dedicated to this with all of the proper checks and casting, but on the flip side, you can use Delegate.CreateDelegate and cache the result. Then, the only real additional cost is reflection required to initially create it. Subsequent runs would be as fast as a normal call (at least in this case. If the type's method body were shorter, it would have the opportunity to be inlined where the delegate would not). Anyway, this is slowly moving to a pointless argument about reflection, so, here's the bottom line: We will add an additional non-generic method overload for these generic ones. This could be done in a few short months, a year, or perhaps even longer. I would prefer months, but sometimes it just can't happen. In the meantime if you decide that you can't live without this, you can easily write code that uses reflection to get this done (it took me all of 10 minutes to write the above code). Any reflection code you write can be viewed as temporary, since we are going to add this additional non-generic method overload (I feel like I can't stress this enough). If you implement the reflection correctly, you will likely see no real measurable gain in performance when you are able to switch to using that non-generic method overload in the future.
        Hide
        Jens Melgaard added a comment -

        Christopher Currens

        No I didn't miss your point, nor did I resist you, I just disagreed with the part where you implied it wasn't that much of a deal (it was only 2 lines of code after all), and then I gave an explanation as to why I didn't see things like that, isn't that common when presenting a point of view?...

        Why do you think I stated at the end that I was OK with this having low priority?

        So where do you check if the type passed into the method is assignable from IAttribute?

        It would be the API's job to verify the type being passed, not the caller. For 2 reasons in this case. It can actually do a more efficient job of it, and the code for it wasn't written in my example, instead I added a note stating that the method should do this verification, how it does this is another thing, what I left in the note assumes we get more often than we add, but I won't get deeper into that because . Secondly, it's the API who knows the requirement on the type passed, not the caller... So that is why you as a caller can settle for those to steps... Otherwise you are making a fragile API in my opinion.

        Show
        Jens Melgaard added a comment - Christopher Currens No I didn't miss your point, nor did I resist you, I just disagreed with the part where you implied it wasn't that much of a deal (it was only 2 lines of code after all), and then I gave an explanation as to why I didn't see things like that, isn't that common when presenting a point of view?... Why do you think I stated at the end that I was OK with this having low priority? So where do you check if the type passed into the method is assignable from IAttribute? It would be the API's job to verify the type being passed, not the caller. For 2 reasons in this case. It can actually do a more efficient job of it, and the code for it wasn't written in my example, instead I added a note stating that the method should do this verification, how it does this is another thing, what I left in the note assumes we get more often than we add, but I won't get deeper into that because . Secondly, it's the API who knows the requirement on the type passed, not the caller... So that is why you as a caller can settle for those to steps... Otherwise you are making a fragile API in my opinion.
        Hide
        Christopher Currens added a comment -

        You've still missed my point. I'm willing to take the blame for not explaining it well enough. I've also misunderstood you and you've misunderstood me again with my question: "So where do you check if the type passed into the method is assignable from IAttribute?". I suppose that's because rhetoric doesn't really translate well across written text?

        At this point, we're wasting time and energy talking about things that don't really matter; I cannot waste either of those things. So instead, I'll just stress it again so that everyone is clear: we are going to add this additional non-generic method overload. Just don't expect it quickly (nothing on the project happens quickly) and prepare for the possibility that it will only be added on a subsequent release.

        Show
        Christopher Currens added a comment - You've still missed my point. I'm willing to take the blame for not explaining it well enough. I've also misunderstood you and you've misunderstood me again with my question: "So where do you check if the type passed into the method is assignable from IAttribute?". I suppose that's because rhetoric doesn't really translate well across written text? At this point, we're wasting time and energy talking about things that don't really matter; I cannot waste either of those things. So instead, I'll just stress it again so that everyone is clear: we are going to add this additional non-generic method overload . Just don't expect it quickly ( nothing on the project happens quickly) and prepare for the possibility that it will only be added on a subsequent release.
        Hide
        Jens Melgaard added a comment - - edited

        I realized that there was a workaround...
        I realized that you wanted to provide examples to that workaround, and it was a fine contribution for those who might need it...
        I realized that you didn't feel that the workaround was that impacting...
        I realized that the solution wasn't high priority...
        I realized that you wanted to implement it because you agree it should be there...
        I realized that the removal stemmed from the java version...
        I realized that you feel this is a pointless discussion...

        I did not realize that you asked a rhetoric question, because across different cultures you shouldn't do that... And especially not over a text medium...'
        (same rule for sarcasm and irony, different cultures perceive those thing differently)

        To get things clear...
        This is what I reacted on from you:
        Two lines of reflection code isn't what I would consider a "whole lot of reflection mess" to get what you want done.

        The bold part is the absolute only part I commented further on, because I disagreed deeply...
        So I still don't think I missed your point, I just disagreed with parts of your answer, and wanted to get the right weight in...

        In any case... I made my case... If you still think I missed your point... Then I don't really care about it anymore...

        Show
        Jens Melgaard added a comment - - edited I realized that there was a workaround... I realized that you wanted to provide examples to that workaround, and it was a fine contribution for those who might need it... I realized that you didn't feel that the workaround was that impacting... I realized that the solution wasn't high priority... I realized that you wanted to implement it because you agree it should be there... I realized that the removal stemmed from the java version... I realized that you feel this is a pointless discussion... I did not realize that you asked a rhetoric question, because across different cultures you shouldn't do that... And especially not over a text medium...' (same rule for sarcasm and irony, different cultures perceive those thing differently) To get things clear... This is what I reacted on from you: Two lines of reflection code isn't what I would consider a "whole lot of reflection mess" to get what you want done. The bold part is the absolute only part I commented further on, because I disagreed deeply... So I still don't think I missed your point, I just disagreed with parts of your answer, and wanted to get the right weight in... In any case... I made my case... If you still think I missed your point... Then I don't really care about it anymore...
        Hide
        Simon Svensson added a comment -

        This should be fixed in revision 1420558, in trunk.

        Show
        Simon Svensson added a comment - This should be fixed in revision 1420558, in trunk.
        Hide
        Christopher Currens added a comment -

        Jens Melgaard Ditto.

        Simon Svensson - Thanks for that. I'm glad you also made the change for HasAttribute as well. I'm debating in my mind whether or not AddAttribute needs one as well. There is AddAttributeImpl which takes in an Attribute class, but all of these other methods take in an interface. Perhaps it might be a good idea to add one of those as well? I can see people using reflection to get only the interfaces and at that point, they wouldn't have a method to use. If you agree that this is something we should also implement, let me know before you start working on any code with it, making that change has a lot of different implications on the code, and I'd like to discuss it before we go in too deep.

        Something I noticed in the code now, is that having dual virtual methods for GetAttribute and HasAttribute is a nightmare. This is the real reason I felt it was necessary to reopen the issue, since there are complications to removing one as virtual. We have to consider what breaking changes this might entail. Since 3.0.3 was released a while ago, marking the generic version as non-virtual would break code that was just updated from the method we removed, as part of the hefty API changes from 2.9.4 to 3.x. However, removing virtual from GetAttribute(Type) would solve backwards compatibility, breaking changes, and perhaps also porting headaches, but would require reflection in order to DRY.

        The internal classes in Lucene use the generic XXXAttribute methods, so we should take care in making changes that will affect the speed of those. In the case of GetAttribute<T> calling the non-generic version, that code winds up making unneeded checks to the variable that the compiler is already doing for free. The speed difference is quite negligible with these two extra checks so this just an example and not an issue. We just need to keep that in mind while we make changes to this class.

        Show
        Christopher Currens added a comment - Jens Melgaard Ditto. Simon Svensson - Thanks for that. I'm glad you also made the change for HasAttribute as well. I'm debating in my mind whether or not AddAttribute needs one as well. There is AddAttributeImpl which takes in an Attribute class , but all of these other methods take in an interface. Perhaps it might be a good idea to add one of those as well? I can see people using reflection to get only the interfaces and at that point, they wouldn't have a method to use. If you agree that this is something we should also implement, let me know before you start working on any code with it, making that change has a lot of different implications on the code, and I'd like to discuss it before we go in too deep. Something I noticed in the code now, is that having dual virtual methods for GetAttribute and HasAttribute is a nightmare. This is the real reason I felt it was necessary to reopen the issue, since there are complications to removing one as virtual. We have to consider what breaking changes this might entail. Since 3.0.3 was released a while ago, marking the generic version as non-virtual would break code that was just updated from the method we removed, as part of the hefty API changes from 2.9.4 to 3.x. However, removing virtual from GetAttribute(Type) would solve backwards compatibility, breaking changes, and perhaps also porting headaches, but would require reflection in order to DRY. The internal classes in Lucene use the generic XXXAttribute methods, so we should take care in making changes that will affect the speed of those. In the case of GetAttribute<T> calling the non-generic version, that code winds up making unneeded checks to the variable that the compiler is already doing for free. The speed difference is quite negligible with these two extra checks so this just an example and not an issue. We just need to keep that in mind while we make changes to this class.
        Hide
        Simon Svensson added a comment -

        Why is it a nightmare to have two virtual methods? Is it an overloading problem, or just an issue if someone does a faulty implementation by overriding GetAttribute<T>() but not GetAttribute(Type)? I'll have to admit that I didn't think about that, but I guess the clean solution (code-wise) would be removing virtual from the generic version, since it just calls the non-generic version anyway. This would break backwards-compatibility, but the changes would be minor (personal bias!) for implementors.

        Removing virtual from the non-generic version would mean that it needed some reflection to call the generic version, so the behavior can be overriden.

        The only way to avoid checking the attribute type twice would be to either 1) duplicate the code, or 2) add an additional method that does the actual fetching, which does not validation of the attribute type. The validation would then be done by the entry method. Both these sounds ugly code-wise.

        Show
        Simon Svensson added a comment - Why is it a nightmare to have two virtual methods? Is it an overloading problem, or just an issue if someone does a faulty implementation by overriding GetAttribute<T>() but not GetAttribute(Type)? I'll have to admit that I didn't think about that, but I guess the clean solution (code-wise) would be removing virtual from the generic version, since it just calls the non-generic version anyway. This would break backwards-compatibility, but the changes would be minor (personal bias!) for implementors. Removing virtual from the non-generic version would mean that it needed some reflection to call the generic version, so the behavior can be overriden. The only way to avoid checking the attribute type twice would be to either 1) duplicate the code, or 2) add an additional method that does the actual fetching, which does not validation of the attribute type. The validation would then be done by the entry method. Both these sounds ugly code-wise.
        Hide
        Christopher Currens added a comment -

        I mean that it's nightmare to have two methods where one calls the other, since one can be overridden and not the other. I feel it's confusing for an implementer to have the ability to override two methods that do the same thing.

        Regarding the check of attribute type twice, I don't think it's necessary. We're looking at a performance decrease of less than 100 ticks for 1000 calls to the method, so it's not at all an issue. I was just bringing it up as a reminder that internally we're using the generic version, so we should either a) not, or b) make sure whatever design decision we make won't adversely affect its performance.

        I agree that the breaking change from removing virtual from the generic version would probably be a minor change. I don't think that there are many people who actually override the {{AttributeSource.XXXAttribute<T>} methods.

        My other main concern was porting difficulty, in terms of our work as maintainers. That's a difficult one to answer, I think, just because I don't know what the future is of these methods (they do look fairly unchanged as of 4.0). Is it in our best interests to stick closer to the Java code and leave the generic method with the actual implementation? This would sacrifice some speed for the non-generic methods in favor of probable ease of porting for us. I think going either route is a gamble for which one would pay off for us...it's probably low risk if we just left it as is, with the generic calling the non-generic.

        Show
        Christopher Currens added a comment - I mean that it's nightmare to have two methods where one calls the other, since one can be overridden and not the other. I feel it's confusing for an implementer to have the ability to override two methods that do the same thing. Regarding the check of attribute type twice, I don't think it's necessary. We're looking at a performance decrease of less than 100 ticks for 1000 calls to the method, so it's not at all an issue. I was just bringing it up as a reminder that internally we're using the generic version, so we should either a) not, or b) make sure whatever design decision we make won't adversely affect its performance. I agree that the breaking change from removing virtual from the generic version would probably be a minor change. I don't think that there are many people who actually override the {{AttributeSource.XXXAttribute<T>} methods. My other main concern was porting difficulty, in terms of our work as maintainers. That's a difficult one to answer, I think, just because I don't know what the future is of these methods (they do look fairly unchanged as of 4.0). Is it in our best interests to stick closer to the Java code and leave the generic method with the actual implementation? This would sacrifice some speed for the non-generic methods in favor of probable ease of porting for us. I think going either route is a gamble for which one would pay off for us...it's probably low risk if we just left it as is, with the generic calling the non-generic.
        Hide
        Jens Melgaard added a comment - - edited

        Christopher Currens

        The internal classes in Lucene use the generic XXXAttribute methods, so we should take care in making changes that will affect the speed of those. In the case of GetAttribute<T> calling the non-generic version, that code winds up making unneeded checks to the variable that the compiler is already doing for free. The speed difference is quite negligible with these two extra checks so this just an example and not an issue. We just need to keep that in mind while we make changes to this class.

        Regarding the check of attribute type twice, I don't think it's necessary. We're looking at a performance decrease of less than 100 ticks for 1000 calls to the method, so it's not at all an issue. I was just bringing it up as a reminder that internally we're using the generic version, so we should either a) not, or b) make sure whatever design decision we make won't adversely affect its performance.

        As sugested, under the circumstance that "GetAttribute" and "HasAttribute" is used more often than adding, AND that the common case is that we find the type in the dictionary, you can basically add the check without having to perform it each time...

        public T GetAttribute<T>() where T : IAttribute
        {
           return (T) GetAttribute(typeof(T));
        } 
        public virtual IAttribute GetAttribute(Type key)
        {
          if (!this.attributes.ContainsKey(key))
          {
            if(!typeof(IAttribute).IsAssignableFrom(t))
              throw new ArgumentException("Type of T must inherit from IAttribute");
        
            throw new ArgumentException("This AttributeSource does not have the attribute '" + key.FullName + "'.");
          }
          else
            return (T) this.attributes[key].Value;
        }
        

        Because I assume that if the type exist in the dictionary, then it actually complies with the IAttribute interface, if it does not exist then you can continue to determine what error to throw... Odd code in a sense...

        Ofc. thats besides the point when we take the other issue (portability from java) into account... I never ported a Java project, so I don't know what tools there is and how they work...

        Show
        Jens Melgaard added a comment - - edited Christopher Currens The internal classes in Lucene use the generic XXXAttribute methods, so we should take care in making changes that will affect the speed of those. In the case of GetAttribute<T> calling the non-generic version, that code winds up making unneeded checks to the variable that the compiler is already doing for free. The speed difference is quite negligible with these two extra checks so this just an example and not an issue. We just need to keep that in mind while we make changes to this class. — Regarding the check of attribute type twice, I don't think it's necessary. We're looking at a performance decrease of less than 100 ticks for 1000 calls to the method, so it's not at all an issue. I was just bringing it up as a reminder that internally we're using the generic version, so we should either a) not, or b) make sure whatever design decision we make won't adversely affect its performance. As sugested, under the circumstance that "GetAttribute" and "HasAttribute" is used more often than adding, AND that the common case is that we find the type in the dictionary, you can basically add the check without having to perform it each time... public T GetAttribute<T>() where T : IAttribute { return (T) GetAttribute(typeof(T)); } public virtual IAttribute GetAttribute(Type key) { if (! this .attributes.ContainsKey(key)) { if (!typeof(IAttribute).IsAssignableFrom(t)) throw new ArgumentException( "Type of T must inherit from IAttribute" ); throw new ArgumentException( "This AttributeSource does not have the attribute '" + key.FullName + "'." ); } else return (T) this .attributes[key].Value; } Because I assume that if the type exist in the dictionary, then it actually complies with the IAttribute interface, if it does not exist then you can continue to determine what error to throw... Odd code in a sense... Ofc. thats besides the point when we take the other issue (portability from java) into account... I never ported a Java project, so I don't know what tools there is and how they work...
        Hide
        Simon Svensson added a comment -

        I feel responsible for fixing my previous somewhat hasty commit. The latest code-snippet will break backward compatibility by making GetAttribute<T> non-virtual. We seem to agree that this is a minor problem. The latest snippet also solves the issue with unnecessary type checks.

        Does it look okay for everyone else? I could commit it as soon as someone shares my view on it.

        Show
        Simon Svensson added a comment - I feel responsible for fixing my previous somewhat hasty commit. The latest code-snippet will break backward compatibility by making GetAttribute<T> non-virtual. We seem to agree that this is a minor problem. The latest snippet also solves the issue with unnecessary type checks. Does it look okay for everyone else? I could commit it as soon as someone shares my view on it.
        Hide
        Christopher Currens added a comment -

        I think that's probably the solution to take. I can't imagine how many people have overriden that method. It's far more likely they've just been using it as normal. If they haven't been, it looks like it would be a pretty simple migration to the other method. I say commit and close out this issue.

        Show
        Christopher Currens added a comment - I think that's probably the solution to take. I can't imagine how many people have overriden that method. It's far more likely they've just been using it as normal. If they haven't been, it looks like it would be a pretty simple migration to the other method. I say commit and close out this issue.
        Hide
        Simon Svensson added a comment -

        I've committed the previously mentioned structure. I also changed the documentation to correctly note that an ArgumentException is thrown when the attribute is missing in the AttributeSource. Could someone verify my fix and perhaps close this issue?

        Show
        Simon Svensson added a comment - I've committed the previously mentioned structure. I also changed the documentation to correctly note that an ArgumentException is thrown when the attribute is missing in the AttributeSource. Could someone verify my fix and perhaps close this issue?
        Hide
        Christopher Currens added a comment -

        Sorry, Simon. I thought I closed this out already. It looks good to me.

        Show
        Christopher Currens added a comment - Sorry, Simon. I thought I closed this out already. It looks good to me.

          People

          • Assignee:
            Unassigned
            Reporter:
            Jens Melgaard
          • Votes:
            0 Vote for this issue
            Watchers:
            5 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development