lucenenet-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Christopher Currens (JIRA)" <>
Subject [jira] [Commented] (LUCENENET-515) bring back TokenStream.GetAttribute(Type).
Date Wed, 12 Dec 2012 18:42:20 GMT


Christopher Currens commented on LUCENENET-515:

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>}

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's
probably low risk if we just left it as is, with the generic calling the non-generic.
> bring back TokenStream.GetAttribute(Type).
> ------------------------------------------
>                 Key: LUCENENET-515
>                 URL:
>             Project: Lucene.Net
>          Issue Type: Improvement
>          Components: Lucene.Net Core
>    Affects Versions: Lucene.Net 3.0.3
>            Reporter: Jens Melgaard
>             Fix For: Lucene.Net 3.6
> 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...
> {code}TokenStream.GetAttribute(typeof(TermAttribute));{code}
> And god I hated that, so now I replaced it with the much more beautiful:
> {code}TokenStream.GetAttribute<TermAttribute>();{code}
> And deleted that old hag of a method taking a Type... And now I am happy...
> BUT!...
> What when...
> {code}
> Type myNotDefinedHereType = GetTypeFromSomeWhere();
> TokenSteam.GetAttribute.... ?????!?!?!?... Uhm... What now???? 
> {code}
> 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...
> {code}
> Type myNotDefinedHereType = GetTypeFromSomeWhere();
> TokenSteam.GetAttribute(myNotDefinedHereType);
> {code}
> 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:
> {code}
> 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;
>     }
> {code}
> Do:
> {code}
> 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;
>     }
> {code}

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:

View raw message