> On March 11, 2011, 3:53 a.m., Boroondas Gupte wrote: > > indra/newview/llnetmap.cpp, line 337 > > <http://codereview.secondlife.com/r/199/diff/1/?file=1187#file1187line337> > > > > Maybe add a short comment here, that this value is meant to be > > overwritten in the loop below it.
Good idea! I'll do so. > On March 11, 2011, 3:53 a.m., Boroondas Gupte wrote: > > indra/newview/llpanelpeople.cpp, lines 161-162 > > <http://codereview.secondlife.com/r/199/diff/1/?file=1188#file1188line161> > > > > For clarity, please rename these variables to dist1_squared and > > dist2_squared, too. Or eliminate them by calling dist_vec_squared right in > > the return statement: > > return dist_vec_squared(item1_pos, me_pos) < > > dist_vec_squared(item2_pos, me_pos); > > > > (A bit long, but still shorter than the lines right above it.) I think I'll go with the idea of remove the extraneous variables. I don't think it makes the code that much less readable or understandable, and it leaves less for the compiler to have to optimize away. (Not that that last is a reason that can stand up on its own; I simply use it as spice for a stronger reason...) > On March 11, 2011, 3:53 a.m., Boroondas Gupte wrote: > > indra/newview/llselectmgr.cpp, lines 6574-6587 > > <http://codereview.secondlife.com/r/199/diff/1/?file=1189#file1189line6574> > > > > Memory reuse is good, I guess, but having variable names that only > > describe the variable's content correctly part of the time bothers me. > > Oz Linden wrote: > Agree with Boroondas here - the variable used inside the loop should be > min_dist_squared, not min_dist. > > Leave the memory optimization to the compiler. No problem. I was just using what I had on hand. :) > On March 11, 2011, 3:53 a.m., Boroondas Gupte wrote: > > indra/newview/llselectmgr.cpp, lines 6589-6594 > > <http://codereview.secondlife.com/r/199/diff/1/?file=1189#file1189line6589> > > > > Off course, this variable already changed meaning during execution > > before your change, so ... meh. > > Oz Linden wrote: > but since you're touching it anyway, fix it QSL: I'm on it! :D - Cron ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://codereview.secondlife.com/r/199/#review444 ----------------------------------------------------------- On March 12, 2011, 6:33 a.m., Cron Stardust wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://codereview.secondlife.com/r/199/ > ----------------------------------------------------------- > > (Updated March 12, 2011, 6:33 a.m.) > > > Review request for Viewer. > > > Summary > ------- > > I looking at the code, trying to find out where/how to add a new feature, > when I tripped across one of these and it lit my mental warning bells off. > Vector distance comparisons should, IMHO, always be done squared. So I did > some greppin, manual analysis, and some careful modification, and here's the > result. > > > This addresses bug VWR-25126. > http://jira.secondlife.com/browse/VWR-25126 > > > Diffs > ----- > > doc/contributions.txt 344d4c6d7d7e > indra/llcharacter/llbvhloader.cpp 344d4c6d7d7e > indra/llmath/tests/llbbox_test.cpp 344d4c6d7d7e > indra/newview/llagent.cpp 344d4c6d7d7e > indra/newview/llfloaterchat.cpp 344d4c6d7d7e > indra/newview/llhudeffectlookat.cpp 344d4c6d7d7e > indra/newview/llhudeffectpointat.cpp 344d4c6d7d7e > indra/newview/llmaniprotate.cpp 344d4c6d7d7e > indra/newview/llmanipscale.cpp 344d4c6d7d7e > indra/newview/llnetmap.cpp 344d4c6d7d7e > indra/newview/llpanelpeople.cpp 344d4c6d7d7e > indra/newview/llselectmgr.cpp 344d4c6d7d7e > indra/newview/llspeakers.cpp 344d4c6d7d7e > indra/newview/llviewerchat.cpp 344d4c6d7d7e > indra/newview/llvoicevivox.cpp 344d4c6d7d7e > indra/newview/llworld.cpp 344d4c6d7d7e > > Diff: http://codereview.secondlife.com/r/199/diff > > > Testing > ------- > > Compiled a test viewer and used it, undertaking some of my normal activities. > Results felt good, but are currently anecdotal. Any suggestions on how to > properly measure this (or even some actual measurement from those already > instrumented to measure these things,) would be great! > > > Thanks, > > Cron > >
_______________________________________________ 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
