[opensource-dev] Review Request: STORM-1352 Crash in LLNearbyChatScreenChannel::showToastsBottom()

Boroondas Gupte sllists at boroon.dasgupta.ch
Mon Jun 20 12:44:49 PDT 2011



> 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
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.secondlife.com/pipermail/opensource-dev/attachments/20110620/d47c6e8d/attachment.htm 


More information about the opensource-dev mailing list