[PATCH] D83834: Add test utility 'split-file'

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/tools/split-file/split-file.cpp:63
+  const char *end = nullptr;
+  int leadingLines = 0;
+};
----------------
Whilst it's probably harmless, I don't think this should be an `int` (and similar comments for other liner number variables, e.g. `lineNo` in `handle`), since it can never be negative. `size_t` or possibly `unsigned` seem more appropriate for the context.


================
Comment at: llvm/tools/split-file/split-file.cpp:96
+      partToBegin[lastPart].end = line.data();
+    auto &cur = res.first->second;
+    if (!i.is_at_eof())
----------------
I'm not sure it's immediately obvious here what the type of `cur` is, so I'd prefer no `auto`.


================
Comment at: llvm/tools/split-file/split-file.cpp:113
+  SmallString<256> partPath;
+  for (auto &it : partToBegin) {
+    std::error_code ec;
----------------
The name `it` implies to me that this is an iterator, but it's actually a pair, right? Perhaps best to change the name to something (e.g. `keyValue` or whatever)


================
Comment at: llvm/tools/split-file/split-file.cpp:117
+    sys::path::append(partPath, output, it.first);
+    if (std::error_code ec =
+            sys::fs::create_directories(sys::path::parent_path(partPath)))
----------------
You already have an `ec` local variable here. Perhaps this should be renamed and/or the earlier `ec` declaration moved to where it is actually used.


================
Comment at: llvm/tools/split-file/split-file.cpp:157
+    fatal(input, ec.message());
+  (void)sys::fs::remove(output, /*IgnoreNonExisting=*/true);
+  return handle(**bufferOrErr, input);
----------------
`sys::fs::remove` returns a `std::error_code`. I'm not sure we want to be ignoring it - I'd expect some sort of error in that case to be printed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83834