----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://codereview.secondlife.com/r/32/#review48 -----------------------------------------------------------
indra/llvfs/lldiriterator.h <http://codereview.secondlife.com/r/32/#comment22> This documentation is pretty good; it would be better if it were done in a way that was more doxygen compatible (putting the documentation of the methods on the methods). The class should have a definition of the glob syntax and matching rules. indra/llvfs/lldiriterator.cpp <http://codereview.secondlife.com/r/32/#comment23> The translation from glob to regex does not allow for escaped glob characters. Suppose I wanted to find a set of file names that contained a '?' character - the glob would need to escape that character as in "*\?*". I don't know if we actually care passionately about that case, but... indra/llvfs/lldiriterator.cpp <http://codereview.secondlife.com/r/32/#comment25> This translation of the '*' glob character may be too general. Most globbing will not match a leading '.' character in the file name (so the glob "*foo" will match the files "afoo" and "bfoo", but not ".foo"). This has the nice side effect of preventing any glob from matching the '.' and '..' directory links in unix-like file systems (which the iterator user should not have to deal with). indra/llvfs/lldiriterator.cpp <http://codereview.secondlife.com/r/32/#comment24> What happens here if braces are not matched? I believe what you'll get is an invalid regexp that will fail later. Probably better to catch it here where a reasonable diagnostic can be produced. If braces != 0 at the bottom (or if it becomes < 0 the '}' case), then the input glob expression was invalid. indra/llvfs/tests/lldir_test.cpp <http://codereview.secondlife.com/r/32/#comment26> If we're going to support the braces matching, test cases should be added for them. - Oz On 2010-12-17 12:46:52, Seth ProductEngine wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://codereview.secondlife.com/r/32/ > ----------------------------------------------------------- > > (Updated 2010-12-17 12:46:52) > > > Review request for Viewer. > > > Summary > ------- > > Fixed LLDir unit test which failed for some complex wildcard combinations. > Added a class implementing directory entries iteration with pattern matching > which is used in unit tests instead of LLDir::getNextFileInDir. > > This code has been run on Linux only. It should be tested under other > platforms and more test cases should be provided. For example changing > directory contents while iterating through it. > > > This addresses bug STORM-550. > http://jira.secondlife.com/browse/STORM-550 > > > Diffs > ----- > > indra/cmake/Boost.cmake 794ad1fc71d1 > indra/llvfs/CMakeLists.txt 794ad1fc71d1 > indra/llvfs/lldiriterator.h PRE-CREATION > indra/llvfs/lldiriterator.cpp PRE-CREATION > indra/llvfs/tests/lldir_test.cpp 794ad1fc71d1 > > Diff: http://codereview.secondlife.com/r/32/diff > > > Testing > ------- > > > Thanks, > > Seth > >
_______________________________________________ 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
