lucenenet-dev mailing list archives

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


Christopher Currens reopened LUCENENET-515:

[~jmelgaard] Ditto.

[~sisve] - 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.
> 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