[PATCH] D83834: Add test utility 'extract'

[George Rimar via Phabricator via llvm-commits]

grimar added inline comments.


================
Comment at: lld/test/ELF/linkerscript/noload.s:1
 # REQUIRES: x86
+# RUN: extract asm %s -o %t.s && extract lds %s -o %t.lds
----------------
This new style in LLD tests looks much better to me!


================
Comment at: llvm/test/tools/extract/no-leading-lines.s:4
+
+# RUN: extract --no-leading-lines input %s | llvm-mc -triple=x86_64 - 2>&1 | FileCheck %s
+
----------------
I wonder if it is better to use a tool that doesn't require "REQUIRES: x86-registered-target".
E.g. it probably could be yaml2obj that reported a syntax error.


================
Comment at: llvm/test/tools/llvm-objcopy/ELF/strip-symbol.test:14
+#--- list1
+bar # bar
+baz # baz
----------------
`#` -> `##` while you are here?


================
Comment at: llvm/tools/extract/extract.cpp:43
+
+LLVM_ATTRIBUTE_NORETURN static void error(const Twine &message) {
+  WithColor::error(errs(), toolName) << message << '\n';
----------------
This is used only once I think. Perhaps it can be merged with the `error()` below:

```
LLVM_ATTRIBUTE_NORETURN static void error(StringRef filename, const Twine &message) {
if (filename.empty())
 ...
else
 ...
exit(1);
```



================
Comment at: llvm/tools/extract/extract.cpp:63
+          line.substr(markerLen - 4).startswith("--- ")))
+      continue;
+    separator = line.substr(0, markerLen);
----------------
This supports `//---`, but doesn't seem you have a test for it?
I'd also add a comment saying it tries to find ``^(.|//)--- <part>`` line.

Perhaps, it is easier to read this piece when it is:
```
     if (line.size() <= markerLen ||
          !line.substr(markerLen - 4).startswith("--- ")))
       continue;
```




================
Comment at: llvm/tools/extract/extract.cpp:68
+      if (partBegin) {
+        error(input, "'" + separator + cur + "' occurs more than once");
+        exit(1);
----------------
test?


================
Comment at: llvm/tools/extract/extract.cpp:69
+        error(input, "'" + separator + cur + "' occurs more than once");
+        exit(1);
+      }
----------------
You do not need this line, `error()` calls `exit(1)` inside.


================
Comment at: llvm/tools/extract/extract.cpp:81
+  if (!partBegin)
+    error(input, "'" + separator + part + "' was not found");
+  if (!partEnd)
----------------
test?


================
Comment at: llvm/tools/extract/extract.cpp:111
+  if (input.empty())
+    error("input filename is not specified");
+  ErrorOr<std::unique_ptr<MemoryBuffer>> bufferOrErr =
----------------
test?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83834