[PATCH] D66613: [support][llvm-objcopy] Add support for shell wildcards

Fri Aug 23 17:14:12 PDT 2019

rupprecht added a comment.

Here are a couple other places using it in the wild:

https://git.ruby-lang.org/ruby.git/tree/template/Makefile.in#n295
https://chromium.googlesource.com/native_client/pnacl-subzero/+/master/pydir/szbuild.py#441
https://github.com/NetBSD/src/blob/trunk/share/mk/bsd.sys.mk#L352

================
Comment at: llvm/lib/Support/Regex.cpp:214
+
+  int BracketDepth = 0;
+  for (unsigned I = 0, E = Wildcard.size(); I < E; ++I) {
----------------
MaskRay wrote:
> jhenderson wrote:
> > Why is BracketDepth signed? It can't ever be negative.
> `BracketDepth` should just be a boolean. There is no nested depth. With the current implementation, the pattern `[[]*` does not match the string `[a`, while in fnmatch(3) (used by GNU objcopy), it matches.
> 
> Windows does not have fnmatch(3). If there were a similar function, Unicode might be its weakness...
> 
> To correctly parse brackets used in glob, I think the bracket parsing code should detect `[:`, `[.` or `[=` (
> collation-related bracket symbols), and skip all characters until the ending symbol is found.
Thanks, I missed that edge case.

I've removed the `BracketDepth` var entirely. I originally did have it as a boolean but it didn't handle the "nested" case of "[[:alpha:]]" (it's not arbitrarily nested, but it is still kind of nested...). It isn't necessary by changing this test case to handle eating all characters until a matching ].

================
Comment at: llvm/lib/Support/Regex.cpp:218
+    switch (Wildcard[I]) {
+      // * matches any number of characters.
+    case '*':
----------------
jhenderson wrote:
> Looks to me like the comments aren't lined up correctly with their case statements.
Fixed -- this indentation level is forced by clang-format, but it looks better after the case statement rather than before it.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66613/new/

https://reviews.llvm.org/D66613