> 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 > >
_______________________________________________ Policies and (un)subscribe information available here: http://wiki.secondlife.com/wiki/OpenSource-Dev Please read the policies before posting to keep unmoderated posting privileges
