[PATCH] D83834: Add test utility 'extract'

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

I wonder if this utility deserves a small number of tests to show the behaviour is exactly what we want? I think it's fine, but it's not obviously correct that the utility is doing what we want.



================
Comment at: llvm/tools/extract/extract.cpp:24
+
+const char tool_name[] = "extract";
+
----------------
`toolName`

Also, should this be derived from `argv[0]`? That way if the tool gets renamed, the error message still uses the actual executable name.


================
Comment at: llvm/tools/extract/extract.cpp:26
+
+static cl::OptionCategory cat("extract Options");
+
----------------
Is there a way of telling clang-tidy to allow a different naming style?


================
Comment at: llvm/tools/extract/extract.cpp:42
+
+LLVM_ATTRIBUTE_NORETURN static void error(Twine message) {
+  WithColor::error(errs(), tool_name) << message << '\n';
----------------
`const Twine &`?


================
Comment at: llvm/tools/extract/extract.cpp:47
+
+LLVM_ATTRIBUTE_NORETURN static void error(StringRef filename, Twine message) {
+  WithColor::error(errs(), tool_name) << filename << ": " << message << '\n';
----------------
Ditto.


================
Comment at: llvm/tools/extract/extract.cpp:53
+static void handle(MemoryBuffer &inputBuf, StringRef input) {
+  const char *docBegin = nullptr, *docEnd = nullptr;
+  int numEmptyLines = 0;
----------------
I might use `fileBegin` and `fileEnd`.


================
Comment at: llvm/tools/extract/extract.cpp:78
+  if (!docBegin)
+    error(input, "'" + separator + part + "' is not found");
+  if (!docEnd)
----------------
was not found


================
Comment at: llvm/tools/extract/extract.cpp:87
+  uint8_t *buf = (*outputBuf)->getBufferStart();
+  std::fill_n(buf, numEmptyLines, '\n');
+  std::copy(docBegin, docEnd, buf + numEmptyLines);
----------------
If I read this correctly, this will print a series of empty lines before the output. I'm not sure I see the benefit of that? What about empty lines after the contents?


================
Comment at: llvm/tools/extract/extract.cpp:101
+    error("input filename is not specified");
+  ErrorOr<std::unique_ptr<MemoryBuffer>> buffer_or_err =
+      MemoryBuffer::getFileOrSTDIN(input);
----------------
`bufferOrErr`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83834