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

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

Post-commit comments since I've been away and spotted a few relatively minor things. I'll post directly on the commit for the llvm-objcopy side, if I see anything there.



================
Comment at: llvm/docs/CommandGuide/llvm-objcopy.rst:135
+  Allow wildcard syntax for symbol-related flags. On by default for
+  section-related flags. Incompatible with --regex.
+
----------------
--regex -> :option:'--regex' (with back-ticks instead of apostrophes).


================
Comment at: llvm/docs/CommandGuide/llvm-objcopy.rst:149
+
+  Additionally, starting a wildcard with '!' will prevent a match, even if
+  another flag matches. For example ``-w -N '*' -N '!x'`` will strip all symbols
----------------
'!' -> ``!`` for consistency? Not sure.


================
Comment at: llvm/docs/CommandGuide/llvm-strip.rst:109
+  Allow wildcard syntax for symbol-related flags. On by default for
+  section-related flags. Incompatible with --regex.
+
----------------
Same comments as llvm-objcopy document.


================
Comment at: llvm/lib/Support/GlobPattern.cpp:118
 
-  // S is something like "foo*". We can use startswith().
-  if (S.endswith("*") && !hasWildcard(S.drop_back())) {
+  // S is something like "foo*", and the "* is not escaped. We can use
+  // startswith().
----------------
Nit: `"*` -> `'*'` ?


================
Comment at: llvm/unittests/Support/GlobPatternTest.cpp:26
+  Expected<GlobPattern> Pat1 = GlobPattern::create("ab*c*def");
+  EXPECT_TRUE((bool)Pat1);
+  EXPECT_TRUE(Pat1->match("abcdef"));
----------------
This and similar cases should at least be ASSERT_TRUE instead of EXPECT_TRUE. If for some reason a failure is hit, the following lines will attempt to dereference an Expected in a failing state and crash the test rather than simply fail it.

Better however would be to use `ASSERT_THAT_EXPECTED(Pat1, Succeeded());` (double-check exact usage, but it's something like that).


================
Comment at: llvm/unittests/Support/GlobPatternTest.cpp:124
+  Expected<GlobPattern> Pat2 = GlobPattern::create("[]");
+  EXPECT_FALSE((bool)Pat2);
+  handleAllErrors(Pat2.takeError(), [&](ErrorInfoBase &EIB) {});
----------------
Similar to above, you can use `ASSERT_THAT_EXPECTED(Pat2, Failed());` to avoid weird behaviour.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66613