lucenenet-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Christopher Currens (JIRA)" <j...@apache.org>
Subject [jira] [Comment Edited] (LUCENENET-515) bring back TokenStream.GetAttribute(Type).
Date Mon, 10 Dec 2012 19:09:21 GMT

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

Christopher Currens edited comment on LUCENENET-515 at 12/10/12 7:09 PM:
-------------------------------------------------------------------------

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.

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

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.
                
      was (Author: ccurrens):
    Jens, you've missed my point.  I was 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.

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

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 {{MethodInfo.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.  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.
                  
> 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
>
> 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

Mime
View raw message