[opensource-dev] Review Request: Storm-1128 Sort the results of using search in the World Map

[Boroondas Gupte]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://codereview.secondlife.com/r/262/#review629
-----------------------------------------------------------



indra/newview/llfloaterworldmap.cpp
<http://codereview.secondlife.com/r/262/#comment602>

    remove trailing whitespace



indra/newview/llfloaterworldmap.cpp
<http://codereview.secondlife.com/r/262/#comment603>

    If this has to be outside of LLFloaterWorldMap::updateSims to make GCC happy, we could as well simplify it to ...
    
    inline bool sort_region_names(std::pair <U64, LLSimInfo*> const& _left, std::pair <U64, LLSimInfo*> const& _right)
    {
    	return (LLStringUtil::compareInsensitive(_left.second->getName(),_right.second->getName()) < 0);
    }
    
    ... and then call std::sort in LLFloaterWorldMap::updateSims with ...
    	std::sort(sim_info_vec.begin(), sim_info_vec.end(), &sort_region_names);
    
    ... i.e. have a function rather than a functor. I guess the only reason for wrapping the function into a struct would be to workaround the lack of method-local functions in C++. (Compare http://www.flipcode.com/archives/Local_Functions_In_C.shtml . Comments on http://stackoverflow.com/questions/2202731/is-there-support-in-c-stl-for-sorting-objects-by-attribute/2202846#2202846 indicate that functors might sometimes/often perform better, but doesn't explain when or why.)
    
    Jonathan Yap said on IRC he prefers the functor (i.e. the variant with the struct) for consistency reasons. (Apparently it's used elsewhere in the code in similar situations.)
    
    If someone knows how to call std::sort so that the functor can be method-local, please speak up. (In that case I'd prefer that and thus keep the struct.) Lambda functions (e.g. from BOOST) would be another idea, but let's not go there unless someone comes up with ready-to-use code.



indra/newview/llfloaterworldmap.cpp
<http://codereview.secondlife.com/r/262/#comment601>

    Consider replacing the type notation
    	const std::pair <U64, LLSimInfo*>&
    be the (equivalent!) notation
    	std::pair <U64, LLSimInfo*> const&
    which, although less 'traditional', is IMO easier to understand. (See http://www.xs4all.nl/~carlo17/cpp/const.qualifier.html )



indra/newview/llfloaterworldmap.cpp
<http://codereview.secondlife.com/r/262/#comment600>

    If we use std::sort, we really should directly #include <algorithm> in this file, even if it already gets included indirectly (via other includes). The only exception I'd make would be if <algorithm> was already (directly) included by the corresponding header file, indra/newview/llfloaterworldmap.h, but that isn't the case here.


- Boroondas


On April 16, 2011, 6:24 a.m., Jonathan Yap wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://codereview.secondlife.com/r/262/
> -----------------------------------------------------------
> 
> (Updated April 16, 2011, 6:24 a.m.)
> 
> 
> Review request for Viewer.
> 
> 
> Summary
> -------
> 
> The results of using the World Map search option are sorted.
> 
> 
> This addresses bug Storm-1128.
>     http://jira.secondlife.com/browse/Storm-1128
> 
> 
> Diffs
> -----
> 
>   doc/contributions.txt a8f868007986 
>   indra/newview/llfloaterworldmap.cpp a8f868007986 
> 
> Diff: http://codereview.secondlife.com/r/262/diff
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Jonathan
> 
>

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