mgravell (64) [Avatar] Offline
#1
3.3.2
> 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

value-types).
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

3.3
> 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);
or:
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!

3.4.4
> (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 (451) [Avatar] Offline
#2
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
#3
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

Marc
mgravell (64) [Avatar] Offline
#4
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 (451) [Avatar] Offline
#5
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
#6
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
#7
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 (451) [Avatar] Offline
#8
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 (451) [Avatar] Offline
#9
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:

<quote>
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.
</quote>


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
#10
Re: Misc thoughts part 2
> ...as 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 (451) [Avatar] Offline
#11
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.