No major objections.

indra/newview/llfloaterworldmap.cpp (Diff revision 1)

CS: sim_info_vec

1. Jonathan Yap April 15, 2011, 11:35 a.m. (April 15, 2011, 11:35 a.m.)

   Changed.

indra/newview/llfloaterworldmap.cpp (Diff revision 1)

I'd move the iterator declaration inside the "for" clause to limit its scope.

1. Jonathan Yap April 15, 2011, 11:35 a.m. (April 15, 2011, 11:35 a.m.)

   Changed.

indra/newview/llfloaterworldmap.cpp (Diff revision 1)

Why do you make this a functor rather than a plain normal function? It's not like we have to keep any internal state or something.

1. Jonathan Yap April 15, 2011, 11:35 a.m. (April 15, 2011, 11:35 a.m.)

   Moved to inside of routine.

2. Boroondas Gupte April 15, 2011, 1:29 p.m. (April 15, 2011, 1:29 p.m.)

   That's a good idea, too, but not what I meant. I was wondering why you are using a class (or struct) with an operator() instead of just a function. std::sort can take either a functor (i.e. a callable object like you're using here) or a function pointer.

3. Jonathan Yap April 15, 2011, 2:04 p.m. (April 15, 2011, 2:04 p.m.)

   Nothing else in this class would make use of this  function so it seemed to make sense to go the struct route (plus when I was trying to figure out how to code this initially the example I found elsewhere in the code was done this way).

4. Boroondas Gupte April 15, 2011, 2:35 p.m. (April 15, 2011, 2:35 p.m.)

   Ah, right. I forgot local function aren't allowed in C++. Otherwise one could have done something like
   	inline bool sortRegionNames(std::pair <U64, LLSimInfo*>& _left, std::pair <U64, LLSimInfo*>& _right)
   	{
   		return(LLStringUtil::compareInsensitive(_left.second->getName(),_right.second->getName()) < 0);
   	}
   
   	std::vector<std::pair <U64, LLSimInfo*> > sim_info_vec(LLWorldMap::getInstance()->getRegionMap().begin(), LLWorldMap::getInstance()->getRegionMap().end());
   	std::sort(sim_info_vec.begin(), sim_info_vec.end(), &sortRegionNames);
   
   So we probably have to either stick to your struct or make sortRegionNames a method or free function.

5. Jonathan Yap April 16, 2011, 4:52 a.m. (April 16, 2011, 4:52 a.m.)

   After fixing the >> to > > issues are you able to compile if the struct I moved from the top of the code into this routine is moved back to the top again?  The other example I found had it that way, and now I am wondering if it was like that to allow linux to compile.

6. Boroondas Gupte April 16, 2011, 9:59 a.m. (April 16, 2011, 9:59 a.m.)

   With storm-1128c.diff (r3 of this review request) the viewer compiles fine, but only because it also changes the arguments of the comparison function to const references. (We discussed this on IRC.) See also http://stackoverflow.com/questions/4084035/stl-sort-issue-with-gcc
   
   I still got some comments about the code, will post them in a review of the r3 diff.

indra/newview/llfloaterworldmap.cpp (Diff revision 1)

Wow, long line. Might want to put the arguments on separate lines.

I tried to compile with your change on Linux (g++ 4.4.5)

indra/newview/llfloaterworldmap.cpp (Diff revision 2)

GCC chokes on the "std::vector<std::pair <U64, LLSimInfo*>>", thinking ">>" is the shift/stream operator. Insert a space to distinguish it from the that:
	std::vector<std::pair <U64, LLSimInfo*> > sim_info_vec(LLWorldMap::getInstance()->getRegionMap().begin(), LLWorldMap::getInstance()->getRegionMap().end());

indra/newview/llfloaterworldmap.cpp (Diff revision 2)

Here, I'm getting

indra/newview/llfloaterworldmap.cpp: In member function ‘void LLFloaterWorldMap::updateSims(bool)’:
indra/newview/llfloaterworldmap.cpp:1498: error: no matching function for call to ‘sort(__gnu_cxx::__normal_iterator<std::pair<long long unsigned int, LLSimInfo*>*, std::vector<std::pair<long long unsigned int, LLSimInfo*>, std::allocator<std::pair<long long unsigned int, LLSimInfo*> > > >, __gnu_cxx::__normal_iterator<std::pair<long long unsigned int, LLSimInfo*>*, std::vector<std::pair<long long unsigned int, LLSimInfo*>, std::allocator<std::pair<long long unsigned int, LLSimInfo*> > > >, LLFloaterWorldMap::updateSims(bool)::SortRegionNames)’

I'm not sure how to fix that. :-\

I tried an #include <algorithm>, but that didn't make this error go away. (But would probably be a good idea anyway.)

indra/newview/llfloaterworldmap.cpp (Diff revision 2)

Here, too, "> >" instead of ">>".

indra/newview/llfloaterworldmap.cpp (Diff revision 3)

remove trailing whitespace

1. Jonathan Yap April 16, 2011, 1:37 p.m. (April 16, 2011, 1:37 p.m.)

   Changed.

indra/newview/llfloaterworldmap.cpp (Diff revision 3)

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 (Diff revision 3)

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 )

1. Jonathan Yap April 16, 2011, 1:36 p.m. (April 16, 2011, 1:36 p.m.)

   Changed.

indra/newview/llfloaterworldmap.cpp (Diff revision 3)

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.

1. Jonathan Yap April 16, 2011, 1:36 p.m. (April 16, 2011, 1:36 p.m.)

   Good observation.  Changed.