mgravell (64) [Avatar] Offline
> PutInList (twice)
was MakeList in original listing 3.3

Listing 3.4
> return value.CompareTo(default(T));
It doesn't even pretend to handle the null case; this means that it could never* return 0 (equal)

for a reference-type (not even with "" vs null for string). Could add struct constraint, but that

would obviously preclude string.
Could perhaps handle null as a special case (which you mention later will get JIT-optimised for

In production code I would use below; it doesn't *visibly* use the IComparable<T> constraint, but

it is used under the bonnet by CreateComparer(), to create a GenericComparer<T> which *does* have

an IComparable<T> constraint.
return Comparer<T>.Default.Compare(value, default(T));
(I know you know this, but IComparable<T> is not a constraint of Comparer<T>, but it is detected

and used)
*=assuming a typical implementation of CompareTo

> Without the extra information these interfaces give us access to, there is little we can > do in

terms of comparisons, other than calling Equals(object) which will result in
> boxing the value we want to compare with when it’s a value type.
vs (later)
> Two classes which are extremely useful when it comes to comparing values are
> EqualityComparer<T> and Comparer<T>
Annoyingly, after reading the first "there is little we can do" bit, I started citing these two as

obvious counterpoints. You rained on my parade! Just a thought, but maybe there is a missing

"(we'll come to this shortly)", or "(yet)" - or some kind of forward reference that makes this

sound a little less defeatist.

listing 3.6
> PartEquals
> GetSubHashCode
Maybe just a style point, but seems odd to be a "Part" in one and a "Sub" in t'other.

> GetSubHashCode
Premature optimisation? You seem to have nipped half of the GenericComparer<T> implementation, but

not all - suggest either:
return EqualityComparer<T>.Default.GetHashCode(obj);
return obj == null ? 0 : obj.GetHashCode();

> You may be concerned that this could end up with a stack overflow if you have a Pair
> where one of the type arguments is itself a Pair. It all works out okay
> though – although you can “nest” the pairs to an arbitrary depth, it can’t actually form
> a loop
Is this cheating?
Pair<object, object> pair = new Pair<object, object>(null,null);
pair.First = pair;
int hash = pair.GetHashCode(); // boom!

> (data binding and PropertyGrid do this, for instance)
Well, strictly speaking they mainly talk to System.ComponentModel via TypeDescriptor (generally in the instance-form, but some of the Type-form exists), which *might* return a reflection-based implementation, but it might not. DataView on an untyped data-set would be an obvious counter-example (since the properties are runtime), but many more exotic things are possible, with interesting effects on performance (search for HyperDescriptor for more info).
(sorry to go on; a pet area of mine...)
jon.skeet (448) [Avatar] Offline
Re: Misc thoughts part 2
MakeList/PutInList: Has been corrected in original, but not updated in MEAP.

Listing 3.4: Good point. I think I'll need to change it not look at comparisons at all, seeing as I'm trying to introduce Comparer<T> about a page later. Alternatively, I could point out that it will fail for null, and then "fix the problem" when I do Comparer<T>. I'll make a note for future study.

Direct comparisons, "little we can do": I've added an extra sentence:
(In fact there are a couple of types to help us in some situations – we’ll come to them in a minute.)

Listing 3.6, GetSubHashCode naming: Changed to GetPartHashCode everywhere.

Listing 3.6, GetSubHashCode implementation: I want to handle nulls (which EqualityComparer doesn't) but avoid boxing for value types which themselves implement IEquatable<T> - I believe EqualityComparer<T>.Default does this. I wouldn't object to changing it to a call to GetHashCode() though if you reckon that's better.

Setting a pair as its own First... hmm. Nasty. I think I'll note that in the text as a situation which you'd come probably up against in any implementation. Alternatively, I might get rid of it altogether smilie

3.4.4: Hmm. Maybe I need a different example smilie

Thanks for all these notes... keep them coming.
mgravell (64) [Avatar] Offline
Re: Misc thoughts part 2
First - sorry for the botched line breaks etc; I read on the train, and notepad is my editor... it doesn't always line up neatly, and frankly I wasn't going to spend ages tarting with it ;-p

> I want to handle nulls (which EqualityComparer doesn't)
Are you sure? I've traced the various concrete implementations (generic/nullable/object/byte) and it seems pretty sound...? Am I missing something? (genuine question)

> (the stack bomb)
OK - it was a pretty unfair demo... GIGO etc ;-p

mgravell (64) [Avatar] Offline
Re: Misc thoughts part 2
> 3.4.4: Hmm. Maybe I need a different example
More generally, it seems a tricky task: how to be accurate *and* concise *and* stay on topic, when huge chunks of the CLI/CLR have big escape-hatches "it behaves like so; oh, unless the class implements <x> in which case it acts differently... and it 2.0 you can do this instead...". How many footnotes are you allowed per page? ;-p

I don't mean to pick at these things, and *absolutely* feel free to ignore me. I won't be in the least offended.
jon.skeet (448) [Avatar] Offline
Re: Misc thoughts part 2
EqualityComparer<T> handling nulls: I was just going by what the documentation said. However, as you say it seems to work fine.

Line breaks etc: not a problem.

Stack bomb: it's still probably worth scrapping the mention. (We're looking to save space anyway.)

Please keep the comments coming - picky is good at this stage. Once it's been published, it might be a slightly different matter!
mgravell (64) [Avatar] Offline
Re: Misc thoughts part 2
3.3.3, listing 3.6
I had some more thoughts on this; mutability and hash:
By having a mutable First and Second, it quietly invites the developer to change them - however, if the pair were already in a dictionary, this could be very bad. You then get into a purist debate of whether it is a documentation issue, vs encapsulation and separation of concerns - i.e. your utility business library shouldn't have to know that the UI has the entries in a dictionary, and likewise your databound UI shouldn't need to care whether the 3rd-party display control internally uses a similar index.

Then my thought process goes:
"make it immutable"
"neat; that fixes the corner-case stack-overflow issue too"
"2 immutable values (simple tuple)?"
"consider making it a struct"
"damn, all we did was rename KeyValuePair<TKey,TValue>"
"ahah! but KVP doesn't implement IEquatable<TKey, TValue>"
"write KeyValuePairEqualityComparer<TKey, TValue> : IEqualityComparer<KeyValuePair><TKey, TValue>>"
"but that doesn't solve the typical UI case which won't know about this comparer"
"damn; perhaps a new struct after all?"

To be honest, you might want to use a bit of licence and sweep this whole thought under a handy rug; on the other hand, to follow up from your earlier observation:
"I can’t remember seeing many bugs where people actually put the wrong type of element in a collection."
If you *haven't* had to debug an issue where an object mutated and broke a hash-match (dictionary), then you've done well. It has certainly bitten me.

Back to the stack overflow; I also realise that in fact I failed to defeat your proposition. Upon re-reading it "where one of the type arguments is itself a Pair" I clearly failed to satisfy the "type arguments" part of this, so your claim still holds water. Fair enough, it can still blow the stack, but only if [ab]used in a really, really stupid way. Do dumb things => expect dumb results. Point to you, I think.
mgravell (64) [Avatar] Offline
Re: Misc thoughts part 2
3.4.4, just after listing 3.11
> validating that however you obtain a reference to a particular type object, there really is only

one such object involved.
Pedant mode; strictly speaking (especially since the vars are typed) this could mean that there

are two objects (instances), and an == operator that compares them on FullName. Personally, if I

care about whether two vars are the same actual instance (rather than "equal in every way that

matters to the Type itself"), then I use ReferenceEquals.


3.4.4, end of; just after listing 3.12
> ###That’s it all### we’re going to cover in the way of advanced features.
That's all? That is all?
jon.skeet (448) [Avatar] Offline
Re: Misc thoughts part 2
Mutability and hash: I'm not going to go into that level of detail, although I could possibly add extra comments in the downloadable code.

I think I'll still just cut the bit about stack overflow - I'm not sure it's worth it.
jon.skeet (448) [Avatar] Offline
Re: Misc thoughts part 2
== operations:
Well, in this case it *does* check that you've got a single type object, as Type doesn't overload ==. There are many ways you can have Type objects which are different but have the same full name. I'm not going to go into that much detail, but I stand by my claim smilie
The docs for Type even call this out:

A Type object that represents a type is unique; that is, two Type object references refer to the same object if and only if they represent the same type. This allows for comparison of Type objects using reference equality.

That's it all:
Thanks, well spotted. Changed to "That's all".

(I'm going to make sure I reply to everything by always making sure I'm the last poster on each thread, btw. That may make for some "no-op" posts, but it'll make it easier to keep track of.)
mgravell (64) [Avatar] Offline
Re: Misc thoughts part 2
> Type doesn't overload...
I know; and I actually meant to change FullName to the AQN - but I'm far enough off-topic already...
Fair enough; I was mainly talking in the more general case ;-p
jon.skeet (448) [Avatar] Offline
Re: Misc thoughts part 2
Agreed about the general case. One of those cases where explaining why I'm right despite the general case would take too much time, unfortunately.