> On June 21, 2011, 7:57 a.m., Alain Linden wrote: > > indra/llvfs/tests/lldiriterator_test.cpp, line 46 > > <http://codereview.secondlife.com/r/357/diff/1/?file=3024#file3024line46> > > > > Personally, I dislike referencing jira items in code comments. Its > > referencing something ephemeral in something that will long out live it. > > Boroondas Gupte wrote: > In general I'd agree, but here my interpretation was that this test was > inspired by a specific issue. Should in that case the whole issue description > be copied into the comment?
The way I see it, the code remains long after the jira is dead and forgotten. As much as possible, code commentary should stand alone. This test makes sense regardless of what jira inspired it, so why bother mentioning it? Years from now a developer reading this code won't care about the jira. That's my opinion. - Alain ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://codereview.secondlife.com/r/357/#review795 ----------------------------------------------------------- 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
