[PATCH] D81614: [llvm][llvm-nm] add TextAPI/MachO support

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

I don't know anything about the tapi/tbd stuff, to speak of, so I can't give any concrete comments, just a number of nits.



================
Comment at: llvm/include/llvm/Object/TapiUniversal.h:57
+    }
+    std::string getInstallName() const {
+      return Parent->Libraries[Index].InstallName.str();
----------------
Nit: missing blank line between this and the previous function.


================
Comment at: llvm/include/llvm/TextAPI/MachO/Architecture.h:49
 
+/// Check if architecture is 64 bit
+bool is64Bit(Architecture);
----------------
Nit: missing full stop.


================
Comment at: llvm/lib/Object/TapiUniversal.cpp:33-42
+  auto FlattenObjectInfo = [this](auto &File) {
+    const auto Name = File->getInstallName();
+    for (const auto Arch : File->getArchitectures())
+      Libraries.emplace_back(Library({Name, Arch}));
+  };
+
+  FlattenObjectInfo(ParsedFile);
----------------
I see `auto` was being used before in various places, but I'm not a fan where it's not clear what the type is (which is the case in almost every instance in these two blocks). However, I'm not familiar with this area of the code base (i.e. the tapi stuff), so feel free to ignore me if this is common style in this area.


================
Comment at: llvm/tools/llvm-nm/llvm-nm.cpp:2101
+  if (TapiUniversal *TU = dyn_cast<TapiUniversal>(&Bin)) {
+    for (auto &I : TU->objects()) {
+      const auto ArchName = I.getArchFlagName();
----------------
Same comment as earlier - don't use `auto` where the type is not obvious from the immediate context.


================
Comment at: llvm/tools/llvm-nm/llvm-nm.cpp:2105
+          ArchFlags.empty() ||
+          any_of(ArchFlags, [&](const auto &Name) { return Name == ArchName; });
+      if (!ShowArch)
----------------
Example why `auto` is bad: I'm assuming Name is actually a `StringRef`, in which case, you don't need the `const &` part of this, since `StringRef` is designed for lightweight copying.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81614