----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://codereview.secondlife.com/r/199/#review544 -----------------------------------------------------------
indra/newview/llselectmgr.cpp <http://codereview.secondlife.com/r/199/#comment475> This comment only really makes sense to those aware of this change. Those reading the code later won't easily be able to make any sense of it. As a rule of thumb, in-code comments should relate to the resulting code and commit messages to the change. If you feel you have to deviate from that, make it explicit. E.g. here, we could write: // Replaces a call to dist_vec(), which uses fsqrtf. Thus that's what we use here, too. F32 min_dist = fsqrtf(min_dist_squared); indra/newview/llselectmgr.cpp <http://codereview.secondlife.com/r/199/#comment476> Even if we decided that it is out of scope to decide what "factor" might have meant here, we can avoid having to place a FIXME comment here by just using a variable name that avoids the term. half_sqrt_of_min_dist is admittedly an ugly name and doesn't tell the reader why we would calculate it, but it is at least truthful and not misleading, so I'd really go for that for now. indra/newview/llselectmgr.cpp <http://codereview.secondlife.com/r/199/#comment477> While we're touching this code anyway, put spaces around (i.e. on both sides of) the binary * operators (multiplication). That makes it easier to visually distinguish them from unary * operators (dereference). - Boroondas On April 4, 2011, 10:34 a.m., Cron Stardust wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://codereview.secondlife.com/r/199/ > ----------------------------------------------------------- > > (Updated April 4, 2011, 10:34 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/llcommon/indra_constants.h 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/llviewermessage.cpp 344d4c6d7d7e > indra/newview/llviewerparceloverlay.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
