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

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added inline comments.


================
Comment at: llvm/test/tools/llvm-objcopy/ELF/wildcard-syntax.test:1
+# RUN: yaml2obj --docnum=1 %s > %t.o
+
----------------
Move the file-level comment before `RUN: `


================
Comment at: llvm/test/tools/llvm-objcopy/ELF/wildcard-syntax.test:3
+
+## This test checks that llvm-objcopy accepts wildcard syntax correctly.
+
----------------
The name is `wildcard-syntax.test` (good). Both `wildcard` and `glob` are referred to in this test. Personally I think `wildcard` is a subset of `glob` and `glob` is a better name here - glob is a POSIX specified concept and the function `fnmatch` that objcopy uses accepts glob, not just wildcard.

Unify the naming here.


================
Comment at: llvm/test/tools/llvm-objcopy/ELF/wildcard-syntax.test:21
+## ! (as a leading character) prevents matches (not dependent on ordering).
+# RUN: llvm-objcopy --remove-section='.???' --remove-section='!.f*' %t.o %t.negmatch.o
+# RUN: llvm-readobj --sections %t.negmatch.o \
----------------
What does `--remove-section='.???' --remove-section='!.f*' --remove-section='.???'` do?


================
Comment at: llvm/test/tools/llvm-objcopy/ELF/wildcard-syntax.test:25
+
+## [a-z] matches a range of characters
+# RUN: llvm-objcopy --remove-section='.[a-c][a-a][q-z]' %t.o %t.range.o
----------------
Full stop.


================
Comment at: llvm/test/tools/llvm-objcopy/ELF/wildcard-syntax.test:26
+## [a-z] matches a range of characters
+# RUN: llvm-objcopy --remove-section='.[a-c][a-a][q-z]' %t.o %t.range.o
+# RUN: llvm-readobj --sections %t.range.o \
----------------
If llvm-objcopy called setlocale, RE Bracket Expression (also used by glob) would be non-portable (behaviors of locales other than the POSIX locale are unspecified).

Just for fun, glibc>=2.27 make `w` and `v` have the same collating order and a `w` test may fail in the Swedish locale (if regex matching is used): https://sourceware.org/bugzilla/show_bug.cgi?id=23393 (Comment #41 said this is basically a wontfix)

In any case, llvm-objcopy does not call setlocale, `w` not used in the test, nor do we use regex, so we are good.


================
Comment at: llvm/test/tools/llvm-objcopy/ELF/wildcard-syntax.test:73
+## ] doesn't close the character class as a first character.
+# RUN: llvm-objcopy --remove-section='[]xyz]' %t.special.o %t.class.2.o
+# RUN: llvm-readobj --sections %t.class.2.o \
----------------
Probably add a section named `z` to enhance the test.


================
Comment at: llvm/tools/llvm-objcopy/CopyConfig.h:115
+  create(StringRef Pattern, MatchStyle MS,
+         std::function<Error(Error)> ErrorCallback);
+
----------------
If ErrorCallback is not stored, consider llvm::function_ref.


================
Comment at: llvm/tools/llvm-objcopy/CopyConfig.h:119
+  bool operator==(StringRef S) const {
+    return R ? return R->match(S) : G ? return G->match(S) : Name == S;
+  }
----------------
Remove some `return`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66613