----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://codereview.secondlife.com/r/357/#review788 -----------------------------------------------------------
doc/contributions.txt <http://codereview.secondlife.com/r/357/#comment780> Please indent issue keys in this file with a single tab, rather than four spaces. doc/contributions.txt <http://codereview.secondlife.com/r/357/#comment779> Current and past practice is that only volunteers are listed in doc/contributions.txt, not LL employees or contractors (like PE employees). I'm not against changing that practice (or getting rid of the file altogether, now that changeset committers better reflect the change authors), but this should probably be discussed first. indra/llvfs/lldiriterator.cpp <http://codereview.secondlife.com/r/357/#comment787> The glob_to_regex funcion looks like it might be useful outside the context of LLDirIterator or even file systems, so maybe it should reside in a source file of its own, or together with other string manipulation stuff. indra/llvfs/lldiriterator.cpp <http://codereview.secondlife.com/r/357/#comment781> +1 for documentation. Bonus points if you turn this into doxygen. Further, start the first sentence with a capital letter and end the last with a period. indra/llvfs/lldiriterator.cpp <http://codereview.secondlife.com/r/357/#comment782> Not related to your change, but I'd find '* 2' more readable than a left-shift. A decent compiler should produce the same code from both, so for performance, it should not matter. indra/llvfs/lldiriterator.cpp <http://codereview.secondlife.com/r/357/#comment784> The second comment line here doesn't start a new sentence, thus should start with a lower case letter. Both lines together are a sentence, so end the second line with a period. indra/llvfs/lldiriterator.cpp <http://codereview.secondlife.com/r/357/#comment783> Indent the regex+= line with 4 tabs and the default line with 3 tabs, to be consistent with surrounding lines. indra/llvfs/lldiriterator.cpp <http://codereview.secondlife.com/r/357/#comment786> Consider surrounding the += operator by a space on each side. (Also in occurrences above.) indra/newview/lllogchat.cpp <http://codereview.secondlife.com/r/357/#comment785> Not introduced by your change, but getting more apparent as more characters get converted to '_': Is it intended that this can lead to filename 'collisions'? I just tried on Aditi and founded groups *Boroondas* and >Boroondas<, and indeed, chat in both gets logged to the same file. - Boroondas On June 20, 2011, 8:21 p.m., Squire Linden wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://codereview.secondlife.com/r/357/ > ----------------------------------------------------------- > > (Updated June 20, 2011, 8:21 p.m.) > > > Review request for Viewer, Oz Linden, Seth ProductEngine, and Alain Linden. > > > Summary > ------- > > Changes to fix CHOP-662. CHOP-662 was caused by the recent addition of > lldiriterator.cpp which is used to iterate over directories. It takes a mask > in the form of a glob string to use as a pattern for which files to iterate > over. > > Unfortunately, as originally implemented, it converted the glob to a regex > pattern without escaping any legal glob characters which are illegal regex > characters. As a result if the passed in mask was an illegal regex it would > fail and llerrs would cause a crash. > > In CHOP-662 this happens whenever a group name has, e.g. a '+' character at > the beginning. When logging in the viewer would attempt to load the group > chat log file which would result in a glob starting with a '+' character and > in turn to an illegal regex in lldiriterator. > > Also, the chat code was doing nothing to ensure that illegal glob characters > were not being passed to the lldiriterator constructor. > > > This addresses bug CHOP-662. > http://jira.secondlife.com/browse/CHOP-662 > > > Diffs > ----- > > doc/contributions.txt e8f2a53c3d6e > indra/llvfs/CMakeLists.txt e8f2a53c3d6e > indra/llvfs/lldiriterator.cpp e8f2a53c3d6e > indra/llvfs/tests/lldiriterator_test.cpp PRE-CREATION > indra/newview/lllogchat.cpp e8f2a53c3d6e > > Diff: http://codereview.secondlife.com/r/357/diff > > > Testing > ------- > > Added a unit test for lldiriterator which checks for construction failures on > examples of the most common group names which were causing the crashes. > > > Thanks, > > Squire > >
_______________________________________________ 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
