[opensource-dev] Review Request: CHOP-619 fix for pre-compiled headers issue

Brad Kittenbrink brad at lindenlab.com
Wed May 4 12:05:11 PDT 2011



> On May 4, 2011, 11:36 a.m., Boroondas Gupte wrote:
> > indra/llxuixml/llxuiparser.h, line 31
> > <http://codereview.secondlife.com/r/289/diff/1/?file=1543#file1543line31>
> >
> >     Any reason to keep this commented out instead of removing it completely?

oops!  No.  good catch!


> On May 4, 2011, 11:36 a.m., Boroondas Gupte wrote:
> > indra/newview/llagent.cpp, lines 279-291
> > <http://codereview.secondlife.com/r/289/diff/1/?file=1545#file1545line279>
> >
> >     I know it's best practice to always set pointers to NULL after deleting, but does that also apply in the destructor? If so, maybe it should also be done for mMouselookModeInSignal and mMouselookModeOutSignal.

Yeah, it doesn't really apply in the destructor as much, but I got in the habit of doing it anyways, since if there's a dangling pointer to this LLAgent instance elsewhere, it could end up making a difference.  You're right that it's a good idea to make mMouselookModeInSignal and mMouselookModeOutSignal consistent.


> On May 4, 2011, 11:36 a.m., Boroondas Gupte wrote:
> > indra/newview/llagent.cpp, lines 292-294
> > <http://codereview.secondlife.com/r/289/diff/1/?file=1545#file1545line292>
> >
> >     While we're editing around here, should this comment still be kept?

Yeah, probably makes sense to clean that up too.


> On May 4, 2011, 11:36 a.m., Boroondas Gupte wrote:
> > indra/newview/llviewerregion.h, line 253
> > <http://codereview.secondlife.com/r/289/diff/1/?file=1565#file1565line253>
> >
> >     Should probably be a const method.

Sounds good, I'll give it a try.


> On May 4, 2011, 11:36 a.m., Boroondas Gupte wrote:
> > indra/newview/llviewerregion.cpp, lines 88-93
> > <http://codereview.secondlife.com/r/289/diff/1/?file=1566#file1566line88>
> >
> >     Comments referring to "I" should probably be signed, so one knows who's speaking.

Heh, I tried to look up who wrote this initially, and hg blame says it was me.  I guess we're in trouble on this one.  Actually I just did the merge in svn, so I suspect I know who wrote it.  I'll look into this.


> On May 4, 2011, 11:36 a.m., Boroondas Gupte wrote:
> > indra/newview/llviewerregion.cpp, lines 131-140
> > <http://codereview.secondlife.com/r/289/diff/1/?file=1566#file1566line131>
> >
> >     Fix indentation.

yup, sounds good.


> On May 4, 2011, 11:36 a.m., Boroondas Gupte wrote:
> > indra/newview/llviewerwindow.h, lines 469-493
> > <http://codereview.secondlife.com/r/289/diff/1/?file=1568#file1568line469>
> >
> >     Any reason to keep these commented out rather than removing them completely?

Well, I had them commented out as I was testing this build, and I think I've convinced myself that they're completely unused.  I want to delete the corresponding code in llviewerwindow.cpp as well to make this complete.  I may leave in gDebugRaycastTexCoord though.


> On May 4, 2011, 11:36 a.m., Boroondas Gupte wrote:
> > indra/newview/llviewerregion.cpp, line 304
> > <http://codereview.secondlife.com/r/289/diff/1/?file=1566#file1566line304>
> >
> >     Consider breaking this long line into several lines.

sounds good


> On May 4, 2011, 11:36 a.m., Boroondas Gupte wrote:
> > indra/newview/llviewerregion.cpp, line 835
> > <http://codereview.secondlife.com/r/289/diff/1/?file=1566#file1566line835>
> >
> >     Please place spaces around binary operator* .

sounds good


> On May 4, 2011, 11:36 a.m., Boroondas Gupte wrote:
> > indra/newview/llviewerregion.cpp, line 1613
> > <http://codereview.secondlife.com/r/289/diff/1/?file=1566#file1566line1613>
> >
> >     Consider using pre-increments instead of post-increments.

sounds good


- Brad


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


On May 3, 2011, 6:22 p.m., Brad Kittenbrink wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://codereview.secondlife.com/r/289/
> -----------------------------------------------------------
> 
> (Updated May 3, 2011, 6:22 p.m.)
> 
> 
> Review request for Viewer.
> 
> 
> Summary
> -------
> 
> Fix for error using precompiled headers with vs2010 and incredibuild 3.60 beta2.
> 
> We'll use this opportunity to remove all the special cases we had for working around this problem.
> 
> 
> This addresses bug CHOP-616.
>     http://jira.secondlife.com/browse/CHOP-616
> 
> 
> 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
> -------
> 
> I tested builds personally on windows and linux although the cmake changes can only affect windows.
> 
> The changes are such that testing the viewer itself is completely unnecessary.  If it builds, it's good.
> 
> 
> Thanks,
> 
> Brad
> 
>

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


More information about the opensource-dev mailing list