indra/llvfs/lldiriterator.h (Diff revision 1)

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

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

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

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

If we're going to support the braces matching, test cases should be added for them.

As noted below, it would be more efficient to do the translation from the glob expression just once in the constructor rather than on each call to the next method.  The constructor should then throw an exception for an invalid glob expression. 

indra/llvfs/lldiriterator.h (Diff revision 2)

Why not use '^' for the negation qualifier here?  It's more familiar (at least to unix users)

1. Seth ProductEngine Jan. 4, 2011, 12:37 p.m. (Jan. 4, 2011, 12:37 p.m.)

   '!' is defined in pattern matching rules on glob(7) man page, but it could be easily replaced with '^' if it is needed.

indra/llvfs/lldiriterator.cpp (Diff revision 2)

It would be better to make the boost::regex a member variable - rather than translate the mask to a regex on every call to next, translate it once in the constructor. 

1. Seth ProductEngine Jan. 4, 2011, 12:37 p.m. (Jan. 4, 2011, 12:37 p.m.)

   Updated in diff r3.

indra/llvfs/lldiriterator.cpp (Diff revision 2)

There should be a unit test for this translation.

1. Seth ProductEngine Jan. 4, 2011, 12:37 p.m. (Jan. 4, 2011, 12:37 p.m.)

   All unit tests for directory iterator depend on the result of this translation. Looks like most common cases are covered. Does it need some additional test cases? what kind of cases should be added?

indra/llvfs/lldiriterator.cpp (Diff revision 2)

This method of tracking escapes and brackets isn't quite correct, I think... consider translating a string input containing an escaped backslash or an escaped bracket.

1. Seth ProductEngine Jan. 4, 2011, 12:37 p.m. (Jan. 4, 2011, 12:37 p.m.)

   Escaping characters like backslash or a square bracket seems to work as expected. 
   For example the pattern "file\\\\[0-9].abc" used as a mask argument for iterator constructor will match files: "file\1.abc", "file\2.abc", etc.
   The bracket can be escaped by a preceding double backslash "\\[".

Just one policy question with this...

This implementation uses llwarns for various errors in the glob expression.

Given that I expect that the glob expression will normally (always?) be hard coded (I hope no one will accept a pattern as input from the user and pass it on unverified), should these use llerrs so that the error cannot be missed (it crashes)?

Other than that, this looks good now.