> On June 16, 2011, 4:23 p.m., Boroondas Gupte wrote: > > indra/newview/llnearbychathandler.cpp, lines 375-382 > > <http://codereview.secondlife.com/r/341/diff/2/?file=2978#file2978line375> > > > > This function's return type should be changed to bool, if it's intended > > for use with std::sort. (And that's how it's being used currently.) > > Oz Linden wrote: > and better yet... there's no excuse in a routine this small for multiple > returns. > Add a local variable and use the else clause. > > bool inOrder; > if ( .... test ... ) > { > inOrder = false; > } > else > { > ... calc ... > inOrder = v1 > v2; > } > return inOrder; > > any half way decent optimizer will put that bool in register anyway, so > there is no performance difference at all and the code is much clearer and > easier to put breakpoints and logging into. > > > Boroondas Gupte wrote: > I must say I find the version with two returns perfectly readable, maybe > specifically because the routine is short. One doesn't easily miss that there > are multiple returns, other than one might when looking at a screen-filling > routine. > > Though, if you do it with if-else, maybe handle the usual case before the > special case (i.e. test for first.get() && second.get). Also, I'd prefer it > the variable to store the return value would be initialized upon definition, > which can even save the else-clause: > > bool inOrder = false; // default return value, in case one or both > toasts don't exist > > if ( .... positive test ... ) > { > ... calc ... > inOrder = v1 > v2; > } > > return inOrder; > > And while I'm bike-shedding, we could get rid of the multiple get() calls > (although those are probably cheap): > > LLToast* toast_1 = first.get(); > LLToast* toast_2 = second.get(); > > bool inOrder = false; // default return value, in case one or both > toasts don't exist > > if (toast_1 && toast_2) // Toasts might already be gone, see STORM-1352. > { > inOrder = toast_1->getTimeLeftToLive() > > toast_2->getTimeLeftToLive(); > } > > return inOrder;
Oz, I'm not a fan of the endless-indentation-single-return style, especially when it comes to a simple case of handling illegal arguments. - Vadim ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://codereview.secondlife.com/r/341/#review759 ----------------------------------------------------------- On June 20, 2011, 6:55 a.m., Vadim ProductEngine wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://codereview.secondlife.com/r/341/ > ----------------------------------------------------------- > > (Updated June 20, 2011, 6:55 a.m.) > > > Review request for Viewer. > > > Summary > ------- > > Apparently, a nearby chat toast got somehow destroyed while still remaining > in the list of active toasts. > Attempt to sort active toasts in showToastsBottom() then triggered the crash. > > I don't know how to reproduce the crash, i.e. force destroying a toast in a > way that its onClose() method (which would remove references to the toast) > isn't called. > So we'll just remove references to the toast whenever it's destroyed. > > > This addresses bug STORM-1352. > http://jira.secondlife.com/browse/STORM-1352 > > > Diffs > ----- > > indra/newview/llnearbychathandler.cpp UNKNOWN > > Diff: http://codereview.secondlife.com/r/341/diff > > > Testing > ------- > > > Thanks, > > Vadim > >
_______________________________________________ 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
