> On May 6, 2011, 3:19 a.m., Boroondas Gupte wrote: > > 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. > > Boroondas Gupte wrote: > Oh, wait, due to the diff file mix-up that assertion was probably for the > CHOP-619 changes, now under review at > https://codereview.secondlife.com/r/299/ , wasn't it? But most changes on > review here /do/ look like they should be ok if it still compiles.
Right. > 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. Totally agreed. > 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.) 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. - Brad ----------------------------------------------------------- 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
