> 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.
>
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;
- Boroondas
-----------------------------------------------------------
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
