> On April 4, 2011, 2:33 p.m., Boroondas Gupte wrote: > > indra/newview/llselectmgr.cpp, lines 6591-6592 > > <http://codereview.secondlife.com/r/199/diff/1-2/?file=1189#file1189line6591> > > > > 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);
The comment is a warning to those who might notice that a few lines further down a different sqrt function is used. Your edition is clearer though. > On April 4, 2011, 2:33 p.m., Boroondas Gupte wrote: > > indra/newview/llselectmgr.cpp, lines 6595-6597 > > <http://codereview.secondlife.com/r/199/diff/2-3/?file=1252#file1252line6595> > > > > 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). How did I miss doing that? :P > On April 4, 2011, 2:33 p.m., Boroondas Gupte wrote: > > indra/newview/llselectmgr.cpp, lines 6594-6597 > > <http://codereview.secondlife.com/r/199/diff/2-3/?file=1252#file1252line6594> > > > > 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. ugh.. still bugs me. I agree that the FIXME is obnoxious, and your variable name (as ugly as I agree it is,) is only 4 characters longer. Lemme try an idea in a parallel review of my own, let me know what you think. - Cron ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://codereview.secondlife.com/r/199/#review544 ----------------------------------------------------------- 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
