No further objections, thus giving my "Ship it". Though, as I know near nothing about the precompiled header technique and just trust you on the assertion that these changes are ok if everything still compiles, it'd probably be good to get a review from at least one more person.

indra/newview/llviewerregion.cpp (Diff revisions 1 - 2)

Tip: If you use tabs for indentation only, and spaces for alignment, stuff like this will look "right" independent of tab display size. (Yes, I know that most text editors can't do that automatically and might even work against you if you try to do it manually. :-\ )

indra/newview/llviewerregion.cpp (Diff revision 2)

> on further thought, is this a bug?
> should it be 0.5 * (mImpl->mLandp->getMinZ() + mImpl->mLandp->getMaxZ()) instead?

Oh, didn't notice that. Then again, I was merely verifying that you don't alter functionality, not whether the present functionality makes any sense.

Looking at the lines above the one in question and the method name, it might indeed well be that this is a bug and that mdV[VZ] should be set to the mean of the mLandp's minZ and maxZ. However,
1) without explanatory code comments and/or a specification of this method, we cannot tell for sure.
2) Even if this is a bug, relying code might to some hackary to compensate for it, which would lead to overcompensation if this is fixed only here. So this would need careful further examination, first. (If this is a bug and isn't compensated for elsewhere, would its effect be observable somehow? If so, how?)
3) Even if this is a bug, fixing it is out of scope for CHOP-624 and should not delay/block it.

Conclusion: Good catch, but file a separate jira issue for tracking investigation and (if needed) fixing of that potential problem.

1. Brad Kittenbrink May 6, 2011, 11:35 a.m. (May 6, 2011, 11:35 a.m.)

   Totally agreed.

2. Boroondas Gupte May 6, 2011, 11:59 a.m. (May 6, 2011, 11:59 a.m.)

   Please link to that new jira once it's filed. I'm curious what'll become of that topic.

3. Brad Kittenbrink May 9, 2011, 1:43 p.m. (May 9, 2011, 1:43 p.m.)

   finally got around to writing this up: https://jira.secondlife.com/browse/VWR-25719

indra/newview/llviewerwindow.cpp (Diff revision 2)

Just to make sure, as 'static' is that silly C++ keyword with 3 different meanings depending on context: Here you're limiting visibility of the constant to file scope, aren't you? Is that the right thing to do with it, rather than placing it in a namespace or class? Should the same be done for some of the other constants around here? (I saw you did it for one more.)

1. Brad Kittenbrink May 6, 2011, 11:47 a.m. (May 6, 2011, 11:47 a.m.)

   Correct, I want to limit them to file scope.  In particular after I had removed the corresponding declarations from the header for a lot of these constants, I knew they weren't being used anywhere else, so making them part of the class's public interface seemed counterproductive.  Ideally I'd like to delete a lot of this kind of code.
   
   Anyways, in my experience, certain optimizations get made to static const values that often do not get made for constants in unnamed namespaces, and the like (although this likely no longer applies with "modern" compilers).  Part of my ongoing build time optimization work in CHOP-609 is to reduce the number of extern symbols that the linker has to deal with, and this is the easiest way to do that in cases where it can be used.

2. Boroondas Gupte May 6, 2011, 11:56 a.m. (May 6, 2011, 11:56 a.m.)

   I see. Makes sense.