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

Boroondas Gupte sllists at boroon.dasgupta.ch
Fri May 6 11:59:18 PDT 2011



> On May 6, 2011, 3:19 a.m., Boroondas Gupte wrote:
> > indra/newview/llviewerwindow.cpp, line 248
> > <http://codereview.secondlife.com/r/289/diff/2/?file=1687#file1687line248>
> >
> >     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.)
> 
> Brad Kittenbrink wrote:
>     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.

I see. Makes sense.


> On May 6, 2011, 3:19 a.m., Boroondas Gupte wrote:
> > indra/newview/llviewerregion.cpp, lines 833-839
> > <http://codereview.secondlife.com/r/289/diff/2/?file=1684#file1684line833>
> >
> >     > 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.
> 
> Brad Kittenbrink wrote:
>     Totally agreed.

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


- Boroondas


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


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/825d8122/attachment-0001.htm 


More information about the opensource-dev mailing list