[
https://issues.apache.org/jira/browse/LUCENENET-515?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13530195#comment-13530195
]
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>} 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.
> bring back TokenStream.GetAttribute(Type).
> ------------------------------------------
>
> Key: LUCENENET-515
> URL: https://issues.apache.org/jira/browse/LUCENENET-515
> 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: http://www.atlassian.com/software/jira
