Re: [lmi] Code review: product editor

[Greg Chicares]

On 2007-3-20 10:51 UTC, Evgeniy Tarassov wrote:
[...]
> I tried to create a patch for each commit. I will send it archived to
> your personal email.

This method seems to be working well for me so far. I've already
applied your first four patches to HEAD, and they all worked just
fine. Now I'd like to pause to discuss the next two, but let me
do that in a separate email. I'm looking at the seventh patch,
after skipping the fifth and sixth, and I suspect that with due
care I can apply it out of order; 'tier_view_editor.cpp', for
instance, has only three differences from HEAD in the seventh
patch, whereas the branch has thirty-one differences, and it's
much easier to consider only three at one time.

I've kept notes on each patch I've applied, and copied them
below. I thought it would be useful to mention things that I
particularly liked, as well as any different ideas I might have,
and things I want to avoid forgetting to do later...and, last
but not least, non-obvious insights ("I made this regrettable
design decision a decade ago, and I know one way of addressing
it that we must avoid"). I've put them all in one email, but can
just as easily write separate messages instead in the future if
that would be more convenient for you.

20070316T1045Z_br_plus_br_remove.patch

In 'tier_view.cpp', I removed <sstream>, which is no longer needed.

20070316T1111Z_remove_dummy_menu.patch

I added a 'TODO' marker: some menus have no menuitems, and we should
remove them unless we can think of menuitems that they should have.
Wendy, any thoughts?

20070316T1130Z_rename_database_swap_workaround.patch

I was going to ask about this:

-    swap_workaround_for_singleton guard(dict_, instance.GetDictionary());
+    swap_workaround_for_singleton(dict_, instance.GetDictionary());

but I see that you've got

+    swap_workaround_for_singleton workaround(dict_, instance.GetDictionary());

in the branch, presumably because of a later change, so I'll commit
that later change to HEAD now. [Here, it was very convenient to have
the branch as well as individual patches: I could look ahead and see
that you had already addressed that issue.]

I think I saw a similar situation the other day with wxBusyCursor;
I added a comment about that in HEAD.

BTW, I like the new 'swap_workaround_for_singleton' name. It's self
documenting. Files like 'ihs_dbdict.hpp' are among our oldest, and use
ghastly names like 'dict_map_val', so thanks for choosing not to coin
another name like that. If the new name seems jarringly long, I'd say
that's perfectly appropriate, for the same reason 'reinterpret_cast'
is: it directs our attention to suboptimal code elsewhere--to the
singleton in 'ihs_dbdict.hpp'. There, caching is extremely important
for runtime performance, but a one-slot cache is not the best possible
idea. We ought to change that when we rewrite the ancient database
code to use xml, and try to make this workaround unnecessary then.

20070316T1230Z_add_workaround_var_names.patch

Okay, I had combined that with the last patch as explained above.