On Jul 16, 2007, at 16:38, Vincent Hennebert wrote:

Hi

This change makes me a bit uneasy. No doubt that this class is clever
and efficient and working, but that's something more to maintain. By
quickly looking at it I couldn't figure out exactly how it is working,
and this is the kind of code that is very easy to modify in a wrong way.

You're probably right about that. Should've made this more of a proposal than just going ahead.

Granted, it's done, working, and it will probably not change much. But,
well, even if a bit inconvenient and probably not the most efficient,
hashmaps were basically doing the thing.

Well, that was the thought. Not so much the HashMaps themselves, but more the look of all the Integers being constructed solely for the purpose of being able to store ints in a Map.

What I mean is that, well, there are already so many other things to
do... Moreover, before an IntMap makes the difference compared to a
HashMap of Integers regarding performance, there are somewhat bigger
architectural changes to perform ;-)

That certainly is a fact!
I just had this still lying around, noticed it while going over a unified diff, and decided to commit it, but it's probably best to wait a while, as I was actually still unsure of how to integrity-test such a thing... :/


Ok, I guess the fun you had while writing this class was a big
motivation for the change ;-) Still.

Hmm, it was indeed fun :-) OTOH, the small binary search loop is basically copied from some other class in the codebase. For the rest, it's pretty much common sense...

That said, you are absolutely correct in questioning this change.
I guess the wisest course of action, release-wise, is to revert this change while it is still little. I'll keep it in the closet to revisit at a later time.

I'll take care of this tomorrow.


Cheers

Andreas

Reply via email to