[PATCH] D84770: [llvm-libtool-darwin] Add support for -arch_only

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/docs/CommandGuide/llvm-libtool-darwin.rst:64-67
+.. option:: -arch_only <architecture>
+
+ Build a static library only for the specified `<architecture>` and ignore all
+ other architectures in the files.
----------------
I should have been paying more attention to this before, but it would be good to have the options listed in alphabetical order, I think.


================
Comment at: llvm/test/tools/llvm-libtool-darwin/cpu-subtype-matching.test:22
+
+
+## armv6.yaml
----------------
Nit: delete extra blank line.


================
Comment at: llvm/test/tools/llvm-libtool-darwin/universal-file-flattening.test:89-90
+
+# INVALID-ARCH: Invalid architecture: arch101
+# INVALID-ARCH-NEXT: Valid architecture names are
+
----------------
Nit: here and in EMPTY-ARCH, add spaces to make the first line's pattern line up with the one below.


================
Comment at: llvm/tools/llvm-libtool-darwin/llvm-libtool-darwin.cpp:110
+        std::errc::invalid_argument,
+        "Invalid architecture: %s\nValid architecture names are %s",
+        ArchitectureName.str().c_str(), OS.str().c_str());
----------------
Error messages should generally start with a lower-case letter (i.e. "Invalid" -> "invalid")


================
Comment at: llvm/tools/llvm-libtool-darwin/llvm-libtool-darwin.cpp:116
+
+// Check that File's architecture [FileCPUType, FileCPUSubtype]
+// matches the architecture specified under -arch_only flag.
----------------
Given that there's no `File` parameter, perhaps thsi should be "a file's"


================
Comment at: llvm/tools/llvm-libtool-darwin/llvm-libtool-darwin.cpp:119
+static bool acceptFileArch(uint32_t FileCPUType, uint32_t FileCPUSubtype) {
+  unsigned int ArchCPUType, ArchCPUSubtype;
+  std::tie(ArchCPUType, ArchCPUSubtype) = MachO::getCPUTypeFromArchitecture(
----------------
`uint32_t` for these, to match the return type of `getCPUTypeFromArchitecture`.


================
Comment at: llvm/tools/llvm-libtool-darwin/llvm-libtool-darwin.cpp:126-128
+  // Currently LLVM doesn't have the constant MachO::CPU_SUBTYPE_ARM64_V8.
+  // When the constant is added, following modification needs to be done for the
+  // case of MachO::CPU_TYPE_ARM64 below:
----------------
Rather than a comment that will be forgotten about and/or ignored, could you not just add the constant?


================
Comment at: llvm/tools/llvm-libtool-darwin/llvm-libtool-darwin.cpp:226
+    NewArchiveMember NM, StringRef FileName, Config C) {
+
+  Expected<std::unique_ptr<MachOUniversalBinary>> BinaryOrErr =
----------------
Nit: delete blank line here.


================
Comment at: llvm/tools/llvm-libtool-darwin/llvm-libtool-darwin.cpp:233
+  auto *UO = BinaryOrErr->get();
+  for (const auto &O : UO->objects()) {
+
----------------
Don't use `auto` here, please.


================
Comment at: llvm/tools/llvm-libtool-darwin/llvm-libtool-darwin.cpp:249
+    if (ArchiveOrError) {
+      consumeError(MachOObjOrErr.takeError());
+      if (Error E = processArchive(Members, **ArchiveOrError, FileName, C))
----------------
Why are you throwing away this error rahter than reporting it earlier?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84770