[PATCH] D63735: [MachOObjectFile]Added Valid Architecture Function

[Shoaib Meenai via Phabricator via llvm-commits]

smeenai added a comment.

Looking good, just a few style nits.



================
Comment at: llvm/lib/Object/MachOObjectFile.cpp:61
+      "armv6",  "armv6m", "armv7",    "armv7em", "armv7k", "armv7m",
+      "armv7s", "arm64",  "arm64_32", "ppc",     "ppc64"};
+
----------------
Nit: add a trailing comma, so the closing brace is on its own line.

I'm not a huge fan of the binpacking clang-format is doing here, but let's stick with what it wants.


================
Comment at: llvm/lib/Object/MachOObjectFile.cpp:2731
+ArrayRef<StringRef> MachOObjectFile::getValidArchs() {
+  return ArrayRef<StringRef>(validArchs);
 }
----------------
Nit: you can just do `return validArchs;` and the ArrayRef will be implicitly constructed.


================
Comment at: llvm/tools/llvm-lipo/llvm-lipo.cpp:100
+    OS << "Invalid architecture: " << ArchitectureName
+       << "\nValid architecture flags are:";
+    ArrayRef<StringRef> valid = MachOObjectFile::getValidArchs();
----------------
Perhaps "architecture names" instead of "architecture flags"? I know cctools lipo says "architecture flags", but their error message also says "unknown architecture specification **flag**"; without that earlier use of "flag", the context isn't as clear here.


================
Comment at: llvm/tools/llvm-lipo/llvm-lipo.cpp:103
+
+    for (auto arch : valid)
+      OS << " " << arch;
----------------
Nit: I'd just inline the call to `MachOObjectFile::getValidArchs()` instead of creating a separate variable.

Super nit: after inlining and removing the variable, you can also remove the newline above this.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63735