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

[Vadim ProductEngine]

> 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;
> 
> Vadim ProductEngine wrote:
>     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.

Boroondas, as far as I remember, there cannot be many active toasts, so no need to write more code here to save a microsecond.


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

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