[opensource-dev] Review Request: CHOP-624 header dependency improvements for build time speedup

[Boroondas Gupte]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://codereview.secondlife.com/r/289/#review682
-----------------------------------------------------------

Ship it!


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
<http://codereview.secondlife.com/r/289/#comment653>

    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
<http://codereview.secondlife.com/r/289/#comment654>

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



indra/newview/llviewerwindow.cpp
<http://codereview.secondlife.com/r/289/#comment655>

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


- Boroondas


On May 5, 2011, 5:21 p.m., Brad Kittenbrink wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://codereview.secondlife.com/r/289/
> -----------------------------------------------------------
> 
> (Updated May 5, 2011, 5:21 p.m.)
> 
> 
> Review request for Viewer.
> 
> 
> Summary
> -------
> 
> Sorry for the big diff here, but reworking a LOT of header dependencies to reduce complexity and help build time.  Started moving stuff out of public interfaces of classes to improve insulation (for example in llviewerregion.h and llagent.h for the biggest examples).
> 
> 
> This addresses bug CHOP-624.
>     http://jira.secondlife.com/browse/CHOP-624
> 
> 
> Diffs
> -----
> 
>   indra/llcommon/llapp.cpp UNKNOWN 
>   indra/llui/llbutton.h UNKNOWN 
>   indra/llui/llfloaterreg.h UNKNOWN 
>   indra/llui/llfocusmgr.h UNKNOWN 
>   indra/llui/llfocusmgr.cpp UNKNOWN 
>   indra/llui/lliconctrl.h UNKNOWN 
>   indra/llui/lllineeditor.h UNKNOWN 
>   indra/llui/llloadingindicator.h UNKNOWN 
>   indra/llui/llmultislider.cpp UNKNOWN 
>   indra/llui/llpanel.h UNKNOWN 
>   indra/llui/llprogressbar.h UNKNOWN 
>   indra/llui/llprogressbar.cpp UNKNOWN 
>   indra/llui/llslider.h UNKNOWN 
>   indra/llui/llstyle.h UNKNOWN 
>   indra/llui/llstyle.cpp UNKNOWN 
>   indra/llui/lltransutil.cpp UNKNOWN 
>   indra/llui/llui.h UNKNOWN 
>   indra/llui/llview.h UNKNOWN 
>   indra/llui/llviewborder.cpp UNKNOWN 
>   indra/llui/llwindowshade.h UNKNOWN 
>   indra/llxuixml/lltrans.h UNKNOWN 
>   indra/llxuixml/lltrans.cpp UNKNOWN 
>   indra/llxuixml/llxuiparser.h UNKNOWN 
>   indra/newview/llagent.h UNKNOWN 
>   indra/newview/llagent.cpp UNKNOWN 
>   indra/newview/llappviewer.cpp UNKNOWN 
>   indra/newview/lleventnotifier.h UNKNOWN 
>   indra/newview/llfloaterland.h UNKNOWN 
>   indra/newview/llfloaterland.cpp UNKNOWN 
>   indra/newview/llfloatersnapshot.cpp UNKNOWN 
>   indra/newview/llfolderviewitem.h UNKNOWN 
>   indra/newview/lllocationhistory.h UNKNOWN 
>   indra/newview/lloutputmonitorctrl.h UNKNOWN 
>   indra/newview/llpanelavatar.cpp UNKNOWN 
>   indra/newview/llpanelgroupgeneral.cpp UNKNOWN 
>   indra/newview/llpanelgrouproles.cpp UNKNOWN 
>   indra/newview/llpreviewgesture.cpp UNKNOWN 
>   indra/newview/llsidepaneliteminfo.cpp UNKNOWN 
>   indra/newview/lltooldraganddrop.cpp UNKNOWN 
>   indra/newview/llviewerchat.cpp UNKNOWN 
>   indra/newview/llviewerkeyboard.h UNKNOWN 
>   indra/newview/llviewermenu.cpp UNKNOWN 
>   indra/newview/llviewerparcelmgr.cpp UNKNOWN 
>   indra/newview/llviewerprecompiledheaders.h UNKNOWN 
>   indra/newview/llviewerregion.h UNKNOWN 
>   indra/newview/llviewerregion.cpp UNKNOWN 
>   indra/newview/llviewertexturelist.h UNKNOWN 
>   indra/newview/llviewerwindow.h UNKNOWN 
>   indra/newview/llviewerwindow.cpp UNKNOWN 
>   indra/newview/llvoavatar.cpp UNKNOWN 
>   indra/newview/llvotree.cpp UNKNOWN 
>   indra/newview/llvovolume.cpp UNKNOWN 
>   indra/newview/llworld.cpp UNKNOWN 
>   indra/newview/tests/llremoteparcelrequest_test.cpp UNKNOWN 
>   indra/newview/tests/llviewerhelputil_test.cpp UNKNOWN 
> 
> Diff: http://codereview.secondlife.com/r/289/diff
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Brad
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.secondlife.com/pipermail/opensource-dev/attachments/20110506/bfa5106c/attachment-0001.htm