[opensource-dev] Review Request: (STORM-550) LLDir::getNextFileInDir fails for some complex wildcard combinations

[Oz Linden]

-----------------------------------------------------------
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
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.secondlife.com/pipermail/opensource-dev/attachments/20101217/4279430c/attachment-0001.htm