Generic WeakReference
Andrés G. Aragoneses
git will only show the modified files, so I'm attaching the file as
is. Please review.
BTW, does it have any sense to have unit tests for this? They would be
the same as the unit tests for the non-generic standard WeakReference.
Thanks,
Andres
PS: I was going to contribute an implementation based on mixed
concepts mentioned in the connect issue and some other pages, but
found that the one from CodeProject was the best one, so I'm assuming
the CodeProject license is compatible with MIT/X11. Anyway this is one
of the cases in which the implementation is so simple that you
shouldn't be able to apply a license to it.
--
Jonathan Pryor
> BTW, does it have any sense to have unit tests for this? They would be
> the same as the unit tests for the non-generic standard WeakReference.
Indeed, but it would at least serve as a sanity check that
WeakReference<T>.Target was the actual value provided to the
constructor...
> PS: I was going to contribute an implementation based on mixed
> concepts mentioned in the connect issue and some other pages, but
> found that the one from CodeProject was the best one, so I'm assuming
> the CodeProject license is compatible with MIT/X11.
It is not. The CPOL has many additional restrictions that MIT/X11 does
not have, e.g. §3.c:
You may otherwise modify Your copy of this Work (excluding the
Articles) in any way to create a Derivative Work, provided that
You insert a prominent notice in each changed file stating how,
when and where You changed that file.
or §5.e:
You may distribute the Executable Files and Source Code only
under the terms of this License, and You must include a copy of,
or the Uniform Resource Identifier for, this License with every
copy of the Executable Files or Source Code You distribute and
ensure that anyone receiving such Executable Files and Source
Code agrees that the terms of this License apply to such
Executable Files and/or Source Code....
If I'm understanding this properly, this is not GPL compatible, and one
of the major points to MIT/X11 is that it's compatible with ~everything.
I'm not even sure how §5.e is supposed to mix with §3.a; how can you mix
this code with your own program, and (presumably) release the resulting
mixed product under your own license, given §5.e?
Not that the CPOL is "bad," it's just not "great" as per my reading.
Worst of all, to me, is that it's not listed in the opensource.org
license list - which is already too long, but at least being listed
there shows that the license has been reviewed by others and is
generally sane:
http://opensource.org/licenses/alphabetical
> Anyway this is one
> of the cases in which the implementation is so simple that you
> shouldn't be able to apply a license to it.
That may be true, but there's still the documentation in that file as
well.
In short, I don't think we can use that implementation, not as-is
anyway. This suggests that something different may be useful, so as to
minimize potential copyright violations (unless you want to suggest that
the minimal WeakReference<T> w/o comments isn't really copyrightable,
which may be true...).
So an alternative idea is this: given that WeakReference.Target,
WeakReference.IsAlive, and WeakReference.TrackResurrection are all
virtual, it *may* be useful to provide a *wrapper* instead of a subclass
so that alternative subclasses can be used, e.g.
class WeakReferenceDecorator<T>
where T : class
{
public WeakReferenceDecorator (WeakReference
targetReference)
{
TargetReference = targetReference;
}
public WeakReference TargetReference {get; private set;}
public bool IsAlive {get {return
TargetReference.IsAlive;}}
public T Target {get {return (T)
TargetReference.Target;}}
public bool TrackResurrection {get {return TargetReference.TrackResurrection;}}
}
static class WeakReferenceDecorator {
public WeakReferenceDecorator<T> CreateReference<T>(T
target)
where T : class
{
return new WeakReferenceDecorator<T>(new WeakReference (target));
}
public WeakReferenceDecorator<T> CreateReference<T>(T target, bool trackResurrection)
where T : class
{
return new WeakReferenceDecorator<T>(new WeakReference (target, trackResurrection));
}
}
Thoughts?
- Jon
"Andrés G. Aragoneses"
Good idea, but I think it's better to encourage using the generic
WeakReference as the first subclass of WeakReference, and then maybe
allow the people to subclass WeakReference<T> (indeed, I'm thinking
about creating a Gendarme rule that warns about using the non-generic
WeakReference and advices to create a generic or use Cadenza's, so this
rule could also detect subclasses of it and advice subclassing the
generic version as well).
So how about simply stripping the docs and committing? (I'm thinking
that it may happen as well that the docs are simply copied from the
original WeakReference MS docs plus a few "T" additions... so the
question here would be "can anyone sublicense to CPOL from a
very-simple-code+some-docs-from-MS?" ;)
I'll work on some simple unit tests as well, thanks for the advice.
Andrés
--
Jonathan Pryor
> So how about simply stripping the docs and committing?
Strip the docs and follow Mono style conventions -- '{' on a new line
for classes, '{' on the same line for properties, ' ' between tokens and
'(', etc., etc.
See:
http://anonsvn.mono-project.com/source/trunk/mcs/class/README
I also wonder if using TTarget instead of T would be appropriate, though
since there's only one type it's likely not necessary...
- Jon
"Andrés G. Aragoneses"
> On Sun, 2010-01-24 at 20:23 +0100, "Andrés G. Aragoneses" wrote:
>> So how about simply stripping the docs and committing?
>
> Strip the docs and follow Mono style conventions -- '{' on a new line
> for classes, '{' on the same line for properties, ' ' between tokens and
> '(', etc., etc.
Done (although, the rule for classes is the opposite ;)
> See:
>
> http://anonsvn.mono-project.com/source/trunk/mcs/class/README
>
> I also wonder if using TTarget instead of T would be appropriate, though
> since there's only one type it's likely not necessary...
I've left it as is, tell me if you want to make the T->TTarget change in
the end. I also added simple unit tests. Can someone commit if it's ok?
(Attached the diff.)
Talking about unit tests, I'm getting 3 failures:
https://bugzillafiles.novell.org/attachment.cgi?id=338835
(I guess this is not expected.)
Andrés
--
Jonathan Pryor
On Tue, 2010-01-26 at 15:04 +0100, "Andrés G. Aragoneses" wrote:
> diff --git a/src/Cadenza/Cadenza/WeakReference.cs b/src/Cadenza/Cadenza/WeakReference.cs
> new file mode 100644
> index 0000000..a9e1d98
> --- /dev/null
> +++ b/src/Cadenza/Cadenza/WeakReference.cs
> @@ -0,0 +1,59 @@
> +//
> +// WeakReference.cs: A generic replacement for the standard WeakReference class.
> +//
> +// Author:
> +// Andrés G. Aragoneses <kno...@gmail.com>
> +//
> +// Based on ideas from:
> +// http://www.codeproject.com/KB/dotnet/generic_WeakReference.aspx
> +// http://connect.microsoft.com/VisualStudio/feedback/ViewFeedback.aspx?FeedbackID=98270
Drop the codeproject.com reference; we don't need to be reminded of CPOL
(and all the questions that may entail).
> +namespace Cadenza
> +{
> + [Serializable]
> + public class WeakReference<T>
> + : WeakReference where T : class {
The ': WeakReference' should go on the same line, 'where' should go on a
new line (indented), and '{' should go on a new line. (See below.)
Also, reviewing the implementation, it sucks. It sucks because of:
WeakReference<string> r = new WeakReference<string>("foo");
WeakReference b = r;
b.Target = new List<string>();
string s = r.Target; // InvalidCastException
Ideally, we would override and provide a new method in the same class.
Unfortunately, C# doesn't allow that. We can however do it in two
steps, by provide a WeakReferenceChecker<T> which inherits WeakReference
and overrides WeakReference.Target to ensure that the types are safe,
and then a WeakReference<T> which inherits WeakReferenceChecker<T>:
[Serializable]
class WeakReferenceChecker<T> : WeakReference
where T : class
{
public WeakReferenceChecker (T target)
: base (target)
{
}
public WeakReferenceChecker (T target, bool trackResurrection)
: base (target, trackResurrection)
{
}
protected WeakReferenceChecker (SerializationInfo info, StreamingContext context)
: base (info, context)
{
}
public override object Target {
get {return base.Target;}
set {
if (!(value is T))
throw new InvalidOperationException();
base.Target = value;
}
}
}
class WeakReference<T> : WeakReferenceChecker<T>
where T : class
{
public WeakReference (T target)
: base (target)
{
}
public WeakReference (T target, bool trackResurrection)
: base (target, trackResurrection)
{
}
protected WeakReference (SerializationInfo info, StreamingContext context)
: base (info, context)
{
}
public new virtual T Target {
get {return (T) base.Target;}
set {base.Target = value;}
}
}
This is an idea I hadn't seen before, and the above test is something
that should be checked in unit tests (along with making sure that
setting Target via WeakReference actually changes the value, as I nearly
forgot that obvious bit...).
> diff --git a/src/Cadenza/Test/Cadenza/WeakReferenceTest.cs b/src/Cadenza/Test/Cadenza/WeakReferenceTest.cs
...
> +namespace Cadenza.Tests {
> +
> + [TestFixture]
> + public class WeakReferenceTest : BaseRocksFixture {
> +
> + [Test]
> + public void TargetBaseCtor ()
> + {
> + var val = new Uri ("file:///someuri/somepath");
> + var wr = new Cadenza.WeakReference<Uri> (val);
> + Assert.AreEqual (val, wr.Target);
Use Assert.AreSame(), not Assert.AreEqual(). AreEqual() checks for
equality (as per object.Equals()). AreSame checks that they're the same
instance (as per object.ReferenceEquals()).
> + }
> +
> + [Test]
> + public void TargetAdvancedCtor ()
> + {
> + var val = new Uri ("file:///someuri/somepath");
> + var wr = new Cadenza.WeakReference<Uri> (val, true);
> + Assert.AreEqual (val, wr.Target);
Ditto.
Plus you should check that WeakReference.TrackResurrection has the
correct value.
Otherwise, looks sane.
- Jon
Jonathan Pryor
> diff --git a/src/Cadenza/Cadenza/WeakReference.cs b/src/Cadenza/Cadenza/WeakReference.cs
> new file mode 100644
> --- /dev/null
> +++ b/src/Cadenza/Cadenza/WeakReference.cs
> @@ -0,0 +1,88 @@
...
> + [Serializable]
> + public class WeakReference<T> : WeakReferenceChecker<T>
> + where T : class {
As mentioned earlier, the '{' should go on a newline, lined up with
'public'. There doesn't need to be a blank line after it either (as the
'{' alone by itself serves as the blank line).
> + public new T Target {
> + get { return (T)base.Target; }
Space between cast: (T) base.Target
> + [Serializable]
> + public class WeakReferenceChecker<T> : WeakReference
> + where T : class {
{ placement
> + public override object Target {
> + get { return base.Target; }
> + set {
> + if (!(value is T))
> + throw new InvalidOperationException ();
We should use an actual description here.
Other than that, looks good!
Do you have a user account on gitorious so that I can add you as a
contributor and let you commit these changes?
Thanks!
- Jon
"Andrés G. Aragoneses"
> On Tue, 2010-01-26 at 20:42 +0100, "Andrés G. Aragoneses" wrote:
>> diff --git a/src/Cadenza/Cadenza/WeakReference.cs b/src/Cadenza/Cadenza/WeakReference.cs
>> new file mode 100644
>> index 0000000..daee245
>> --- /dev/null
>> +++ b/src/Cadenza/Cadenza/WeakReference.cs
>> @@ -0,0 +1,88 @@
> ...
>> + [Serializable]
>> + public class WeakReference<T> : WeakReferenceChecker<T>
>> + where T : class {
>
> As mentioned earlier, the '{' should go on a newline, lined up with
> 'public'. There doesn't need to be a blank line after it either (as the
> '{' alone by itself serves as the blank line).
As mentioned earlier, that doesn't align with the mono coding
guidelines. See [1] (see the last example, "class X : Y") and [2].
>> + public new T Target {
>> + get { return (T)base.Target; }
>
> Space between cast: (T) base.Target
This is not a requirement that is in the guidelines, and has been
already discussed on the mailing-list in the past [3] as not having a
concrete rule.
>> + [Serializable]
>> + public class WeakReferenceChecker<T> : WeakReference
>> + where T : class {
>
> { placement
Same as above.
>> + public override object Target {
>> + get { return base.Target; }
>> + set {
>> + if (!(value is T))
>> + throw new InvalidOperationException ();
>
> We should use an actual description here.
Ok, how about 'new IOE ("Target should be of type " + typeof(T).Name)'
> Other than that, looks good!
>
> Do you have a user account on gitorious so that I can add you as a
> contributor and let you commit these changes?
I just registered, my username is "knocte".
Thanks,
Andres
[1] http://anonsvn.mono-project.com/source/trunk/mcs/class/README
[2] http://www.mono-project.com/Coding_Guidelines
[3]
http://lists.ximian.com/archives/public/mono-devel-list/2009-May/032109.html
Jonathan Pryor
> El 27/01/10 18:20, Jonathan Pryor escribió:
> > 'public'. There doesn't need to be a blank line after it either (as the
> > '{' alone by itself serves as the blank line).
>
> As mentioned earlier, that doesn't align with the mono coding
> guidelines. See [1] (see the last example, "class X : Y") and [2].
Somehow, I missed that. Also, [1] doesn't address multi-line class base
and interface lists, and when it spans multiple lines I think it looks
more aesthetically pleasing if it's on a new line. :)
>
> >> + public new T Target {
> >> + get { return (T)base.Target; }
> >
> > Space between cast: (T) base.Target
>
> This is not a requirement that is in the guidelines, and has been
> already discussed on the mailing-list in the past [3] as not having a
> concrete rule.
Fair enough. Keep it.
> >> + public override object Target {
> >> + get { return base.Target; }
> >> + set {
> >> + if (!(value is T))
> >> + throw new InvalidOperationException ();
> >
> > We should use an actual description here.
>
> Ok, how about 'new IOE ("Target should be of type " + typeof(T).Name)'
string.Format ("Target 'value' should be of a type implicitly convertible to {0}.",
typeof (T).Name);
> I just registered, my username is "knocte".
You've been added as a contributor. Please commit these to master.
Thanks!
- Jon