[NFC][llvm-libtool-darwin] Encapsulate the process of adding a new member in a class

[Roger]

Summary

Here we are refactoring the code to encapsulate data into classes. This allows
us to avoid passing the same objects through many functions that don't directly
use them. Now, functions that need to access data can do so from the class
state.

[Roger]

Some tests are failing. Will fix.

[Roger]

Failing tests appear unrelated to this change. I ran the tests locally and did not see the same failures.

[smeenai]

Note that this is in preparation for D113130 (the next diff in the stack).

[jhenderson]

Couple of small suggestions to improve the moved code whilst you're moving it. Otherwise, LGTM, though probably best wait for the later patches to be approved before landing this.

llvm/tools/llvm-libtool-darwin/llvm-libtool-darwin.cpp

312–315

I'd probably avoid the "NM" acronym here, as I initially conflated that with the llvm-nm tool for some reason. I'd suggest just calling it NewMemberOrErr and then adding a NewMember variable immediately after the check which is just the dereferenced version, something like the inline edit. The second variable provides a shorter name, and also makes it clear that this has been checked already (since by the time you get to the end of this method, it's quite a long time since you did the check - someone reading the bit in isolation would have to go back and check it).

334–336

You could simplify this to the inline edit.

[alexander-shaposhnikov]

llvm/tools/llvm-libtool-darwin/llvm-libtool-darwin.cpp

305

not sure this class adds much value, the name "AddMember" doesn't seem to be a good name for a class either.
There are multiple ways how refactor the existing code (but they would require deeper changes than what the current diff does) and clean up the API but this class doesn't seem to introduce a good abstraction.

[thevinster]

Title should also be more specific. "Refactor code" is too generic and doesn't say much about what you are introducing here and why it's needed.

[Roger]

Address some comments.

[Roger]

llvm/tools/llvm-libtool-darwin/llvm-libtool-darwin.cpp

312–315

I made the suggested changes in D113215 so that we can maintain the changes in this diff as a "moved from" change.

334–336

I made the suggested changes in D113301 to maintain this change as a whitespace change.

[Roger]

In D113127#3110741, @thevinster wrote:

Title should also be more specific. "Refactor code" is too generic and doesn't say much about what you are introducing here and why it's needed.

Thanks for pointing that out! I've updated the title.

[Roger]

Rebasing.

[Roger]

Rebasing.

[Roger]

Rebase

[alexander-shaposhnikov]

llvm/tools/llvm-libtool-darwin/llvm-libtool-darwin.cpp

312–315

I'm sorry, but this class doesn't appear to solve the problem that it declares to solve (encapsulate ... ), that's why I'm opposed to these changes.
To begin with, this is a class without state, what it does - it saves references to some arguments of the functions but the real mutable state (FileBuffers, MembersPerArchitectureMap) is still exposed, also its usage (lines 467, 468 in llvm-libtool-darwin.cpp) suggests that a function should suffice. The change appears to be somewhat mechanical - the class doesn't introduce a good abstraction, so the new version doesn't seem to be better than the existing code, instead, it appears to be more confusing.

[Roger]

Try a new approach

[Roger]

Add comments

[Roger]

Update comment

[Roger]

Rebase

[Roger]

Renaming

[Roger]

Formatting

[Roger]

llvm/tools/llvm-libtool-darwin/llvm-libtool-darwin.cpp

312–315

I redid the refactoring with your comments in mind. The new MembersBuilder class now completely owns the construction and population of the MembersPerArchitectureMap. That and its memory buffers are stored in MembersData which encapsulates the final output of all the processes of MembersBuilder.

Finally, the MembersBuilder class retains the benefit I was going for at the start which was to stop passing around many objects through many functions and just have them accessible as class members. This makes D113130 to be a simpler change :).

[jhenderson]

Looks generally fine to me, although I haven't looked in depth. @alexander-shaposhnikov?

[smeenai]

LGTM

llvm/tools/llvm-libtool-darwin/llvm-libtool-darwin.cpp

 #include "llvm/Object/ObjectFile.h"
 #include "llvm/Support/CommandLine.h"
 #include "llvm/Support/InitLLVM.h"
 #include "llvm/Support/LineIterator.h"
 #include "llvm/Support/VirtualFileSystem.h"
 #include "llvm/Support/WithColor.h"
 #include "llvm/TextAPI/Architecture.h"
 #include <map>
+#include <type_traits>
 
 using namespace llvm;
 using namespace llvm::object;
 
 static LLVMContext LLVMCtx;
 
 typedef std::map<uint64_t, std::vector<NewArchiveMember>>
     MembersPerArchitectureMap;
case MachO::CPU_TYPE_X86_64:
     // We consider CPUSubtype only for the above 4 CPUTypes to match cctools'
     // libtool behavior.
     return static_cast<uint64_t>(CPUType) << 32 | CPUSubtype;
   default:
     return CPUType;
   }
 }
 
+// MembersData is an organized collection of members.
+struct MembersData {
+  // MembersPerArchitectureMap is a mapping from CPU architecture to a list of
+  // members.
+  MembersPerArchitectureMap MembersPerArchitecture;
+  std::vector<std::unique_ptr<MemoryBuffer>> FileBuffers;
+
+  static_assert(!std::is_copy_constructible<NewArchiveMember>::value,
+                "MembersPerArchitecture has a dependency on FileBuffers so it "
+                "should not be able to be copied on its own without "
+                "FileBuffers.");
+  static_assert(!std::is_copy_assignable<NewArchiveMember>::value,
+                "MembersPerArchitecture has a dependency on FileBuffers so it "
+                "should not be able to be copied on its own without "
+                "FileBuffers.");
+};
+
+// MembersBuilder collects and organizes all members from the files provided by
+// the user.
+class MembersBuilder {
+public:
+  MembersBuilder(const Config &C) : C(C) {}
+
+  Expected<MembersData> build() {
+    for (StringRef FileName : InputFiles)
+      if (Error E = addMember(FileName))
+        return std::move(E);
+
+    if (!ArchType.empty()) {
+      uint64_t ArchCPUID = getCPUID(C.ArchCPUType, C.ArchCPUSubtype);
+      if (Data.MembersPerArchitecture.find(ArchCPUID) ==
+          Data.MembersPerArchitecture.end())
+        return createStringError(std::errc::invalid_argument,
+                                 "no library created (no object files in input "
+                                 "files matching -arch_only %s)",
+                                 ArchType.c_str());
+    }
+    return std::move(Data);
+  }
+
+private:
   // Check that a file's architecture [FileCPUType, FileCPUSubtype]
   // matches the architecture specified under -arch_only flag.
-static bool acceptFileArch(uint32_t FileCPUType, uint32_t FileCPUSubtype,
-                           const Config &C) {
+  bool acceptFileArch(uint32_t FileCPUType, uint32_t FileCPUSubtype) {
     if (C.ArchCPUType != FileCPUType)
       return false;
 
     switch (C.ArchCPUType) {
     case MachO::CPU_TYPE_ARM:
     case MachO::CPU_TYPE_ARM64_32:
     case MachO::CPU_TYPE_X86_64:
       return C.ArchCPUSubtype == FileCPUSubtype;
 
     case MachO::CPU_TYPE_ARM64:
       if (C.ArchCPUSubtype == MachO::CPU_SUBTYPE_ARM64_ALL)
         return FileCPUSubtype == MachO::CPU_SUBTYPE_ARM64_ALL ||
                FileCPUSubtype == MachO::CPU_SUBTYPE_ARM64_V8;
       else
         return C.ArchCPUSubtype == FileCPUSubtype;
 
     default:
       return true;
     }
   }
 
-static Error verifyAndAddMachOObject(MembersPerArchitectureMap &Members,
-                                     NewArchiveMember Member, const Config &C) {
+  Error verifyAndAddMachOObject(NewArchiveMember Member) {
     auto MBRef = Member.Buf->getMemBufferRef();
     Expected<std::unique_ptr<object::ObjectFile>> ObjOrErr =
         object::ObjectFile::createObjectFile(MBRef);
 
     // Throw error if not a valid object file.
     if (!ObjOrErr)
       return createFileError(Member.MemberName, ObjOrErr.takeError());
 
     // Throw error if not in Mach-O format.
     if (!isa<object::MachOObjectFile>(**ObjOrErr))
       return createStringError(std::errc::invalid_argument,
                                "'%s': format not supported",
                                Member.MemberName.data());
 
     auto *O = dyn_cast<MachOObjectFile>(ObjOrErr->get());
     uint32_t FileCPUType, FileCPUSubtype;
     std::tie(FileCPUType, FileCPUSubtype) = MachO::getCPUTypeFromArchitecture(
         MachO::getArchitectureFromName(O->getArchTriple().getArchName()));
 
     // If -arch_only is specified then skip this file if it doesn't match
     // the architecture specified.
-  if (!ArchType.empty() && !acceptFileArch(FileCPUType, FileCPUSubtype, C)) {
+    if (!ArchType.empty() && !acceptFileArch(FileCPUType, FileCPUSubtype)) {
       return Error::success();
     }
 
     if (!NoWarningForNoSymbols && O->symbols().empty())
       WithColor::warning() << Member.MemberName + " has no symbols\n";
 
     uint64_t FileCPUID = getCPUID(FileCPUType, FileCPUSubtype);
-  Members[FileCPUID].push_back(std::move(Member));
+    Data.MembersPerArchitecture[FileCPUID].push_back(std::move(Member));
     return Error::success();
   }
 
-static Error verifyAndAddIRObject(MembersPerArchitectureMap &Members,
-                                  NewArchiveMember Member, const Config &C) {
+  Error verifyAndAddIRObject(NewArchiveMember Member) {
     auto MBRef = Member.Buf->getMemBufferRef();
     Expected<std::unique_ptr<object::IRObjectFile>> IROrErr =
         object::IRObjectFile::create(MBRef, LLVMCtx);
 
     // Throw error if not a valid IR object file.
     if (!IROrErr)
       return createFileError(Member.MemberName, IROrErr.takeError());
 
     Triple TT = Triple(IROrErr->get()->getTargetTriple());
 
     Expected<uint32_t> FileCPUTypeOrErr = MachO::getCPUType(TT);
     if (!FileCPUTypeOrErr)
       return FileCPUTypeOrErr.takeError();
 
     Expected<uint32_t> FileCPUSubTypeOrErr = MachO::getCPUSubType(TT);
     if (!FileCPUSubTypeOrErr)
       return FileCPUSubTypeOrErr.takeError();
 
     // If -arch_only is specified then skip this file if it doesn't match
     // the architecture specified.
     if (!ArchType.empty() &&
-      !acceptFileArch(*FileCPUTypeOrErr, *FileCPUSubTypeOrErr, C)) {
+        !acceptFileArch(*FileCPUTypeOrErr, *FileCPUSubTypeOrErr)) {
       return Error::success();
     }
 
     uint64_t FileCPUID = getCPUID(*FileCPUTypeOrErr, *FileCPUSubTypeOrErr);
-  Members[FileCPUID].push_back(std::move(Member));
+    Data.MembersPerArchitecture[FileCPUID].push_back(std::move(Member));
     return Error::success();
   }
 
-static Error addChildMember(MembersPerArchitectureMap &Members,
-                            const object::Archive::Child &M, const Config &C) {
+  Error addChildMember(const object::Archive::Child &M) {
     Expected<NewArchiveMember> NewMemberOrErr =
         NewArchiveMember::getOldMember(M, C.Deterministic);
     if (!NewMemberOrErr)
       return NewMemberOrErr.takeError();
     auto &NewMember = *NewMemberOrErr;
 
     file_magic Magic = identify_magic(NewMember.Buf->getBuffer());
 
     if (Magic == file_magic::bitcode)
-    return verifyAndAddIRObject(Members, std::move(NewMember), C);
+      return verifyAndAddIRObject(std::move(NewMember));
 
-  return verifyAndAddMachOObject(Members, std::move(NewMember), C);
+    return verifyAndAddMachOObject(std::move(NewMember));
   }
 
-static Error processArchive(MembersPerArchitectureMap &Members,
-                            object::Archive &Lib, StringRef FileName,
-                            const Config &C) {
+  Error processArchive(object::Archive &Lib, StringRef FileName) {
     Error Err = Error::success();
     for (const object::Archive::Child &Child : Lib.children(Err))
-    if (Error E = addChildMember(Members, Child, C))
+      if (Error E = addChildMember(Child))
         return createFileError(FileName, std::move(E));
     if (Err)
       return createFileError(FileName, std::move(Err));
 
     return Error::success();
   }
 
-static Error
-addArchiveMembers(MembersPerArchitectureMap &Members,
-                  std::vector<std::unique_ptr<MemoryBuffer>> &ArchiveBuffers,
-                  NewArchiveMember NewMember, StringRef FileName,
-                  const Config &C) {
+  Error addArchiveMembers(NewArchiveMember NewMember, StringRef FileName) {
     Expected<std::unique_ptr<Archive>> LibOrErr =
         object::Archive::create(NewMember.Buf->getMemBufferRef());
     if (!LibOrErr)
       return createFileError(FileName, LibOrErr.takeError());
 
-  if (Error E = processArchive(Members, **LibOrErr, FileName, C))
+    if (Error E = processArchive(**LibOrErr, FileName))
       return E;
 
-  // Update vector ArchiveBuffers with the MemoryBuffers to transfer
+    // Update vector FileBuffers with the MemoryBuffers to transfer
     // ownership.
-  ArchiveBuffers.push_back(std::move(NewMember.Buf));
+    Data.FileBuffers.push_back(std::move(NewMember.Buf));
     return Error::success();
   }
 
-static Error addUniversalMembers(
-    MembersPerArchitectureMap &Members,
-    std::vector<std::unique_ptr<MemoryBuffer>> &UniversalBuffers,
-    NewArchiveMember NewMember, StringRef FileName, const Config &C) {
+  Error addUniversalMembers(NewArchiveMember NewMember, StringRef FileName) {
     Expected<std::unique_ptr<MachOUniversalBinary>> BinaryOrErr =
         MachOUniversalBinary::create(NewMember.Buf->getMemBufferRef());
     if (!BinaryOrErr)
       return createFileError(FileName, BinaryOrErr.takeError());
 
     auto *UO = BinaryOrErr->get();
     for (const MachOUniversalBinary::ObjectForArch &O : UO->objects()) {
 
       Expected<std::unique_ptr<MachOObjectFile>> MachOObjOrErr =
           O.getAsObjectFile();
       if (MachOObjOrErr) {
         NewArchiveMember NewMember =
             NewArchiveMember(MachOObjOrErr->get()->getMemoryBufferRef());
         NewMember.MemberName = sys::path::filename(NewMember.MemberName);
 
-      if (Error E = verifyAndAddMachOObject(Members, std::move(NewMember), C))
+        if (Error E = verifyAndAddMachOObject(std::move(NewMember)))
           return E;
         continue;
       }
 
       Expected<std::unique_ptr<IRObjectFile>> IRObjectOrError =
           O.getAsIRObject(LLVMCtx);
       if (IRObjectOrError) {
         // A universal file member can be a MachOObjectFile, an IRObject or an
-      // Archive. In case we can successfully cast the member as an IRObject, it
-      // is safe to throw away the error generated due to casting the object as
-      // a MachOObjectFile.
+        // Archive. In case we can successfully cast the member as an IRObject,
+        // it is safe to throw away the error generated due to casting the
+        // object as a MachOObjectFile.
         consumeError(MachOObjOrErr.takeError());
 
         NewArchiveMember NewMember =
             NewArchiveMember(IRObjectOrError->get()->getMemoryBufferRef());
         NewMember.MemberName = sys::path::filename(NewMember.MemberName);
 
-      if (Error E = verifyAndAddIRObject(Members, std::move(NewMember), C))
+        if (Error E = verifyAndAddIRObject(std::move(NewMember)))
           return E;
         continue;
       }
 
       Expected<std::unique_ptr<Archive>> ArchiveOrError = O.getAsArchive();
       if (ArchiveOrError) {
         // A universal file member can be a MachOObjectFile, an IRObject or an
-      // Archive. In case we can successfully cast the member as an Archive, it
-      // is safe to throw away the error generated due to casting the object as
-      // a MachOObjectFile.
+        // Archive. In case we can successfully cast the member as an Archive,
+        // it is safe to throw away the error generated due to casting the
+        // object as a MachOObjectFile.
         consumeError(MachOObjOrErr.takeError());
         consumeError(IRObjectOrError.takeError());
 
-      if (Error E = processArchive(Members, **ArchiveOrError, FileName, C))
+        if (Error E = processArchive(**ArchiveOrError, FileName))
           return E;
         continue;
       }
 
       Error CombinedError = joinErrors(
           ArchiveOrError.takeError(),
           joinErrors(IRObjectOrError.takeError(), MachOObjOrErr.takeError()));
       return createFileError(FileName, std::move(CombinedError));
     }
 
-  // Update vector UniversalBuffers with the MemoryBuffers to transfer
+    // Update vector FileBuffers with the MemoryBuffers to transfer
     // ownership.
-  UniversalBuffers.push_back(std::move(NewMember.Buf));
+    Data.FileBuffers.push_back(std::move(NewMember.Buf));
     return Error::success();
   }
 
-static Error addMember(MembersPerArchitectureMap &Members,
-                       std::vector<std::unique_ptr<MemoryBuffer>> &FileBuffers,
-                       StringRef FileName, const Config &C) {
+  Error addMember(StringRef FileName) {
     Expected<NewArchiveMember> NewMemberOrErr =
         NewArchiveMember::getFile(FileName, C.Deterministic);
     if (!NewMemberOrErr)
       return createFileError(FileName, NewMemberOrErr.takeError());
     auto &NewMember = *NewMemberOrErr;
 
     // For regular archives, use the basename of the object path for the member
     // name.
     NewMember.MemberName = sys::path::filename(NewMember.MemberName);
     file_magic Magic = identify_magic(NewMember.Buf->getBuffer());
 
     // Flatten archives.
     if (Magic == file_magic::archive)
-    return addArchiveMembers(Members, FileBuffers, std::move(NewMember),
-                             FileName, C);
+      return addArchiveMembers(std::move(NewMember), FileName);
 
     // Flatten universal files.
     if (Magic == file_magic::macho_universal_binary)
-    return addUniversalMembers(Members, FileBuffers, std::move(NewMember),
-                               FileName, C);
+      return addUniversalMembers(std::move(NewMember), FileName);
 
     // Bitcode files.
     if (Magic == file_magic::bitcode)
-    return verifyAndAddIRObject(Members, std::move(NewMember), C);
+      return verifyAndAddIRObject(std::move(NewMember));
 
-  return verifyAndAddMachOObject(Members, std::move(NewMember), C);
+    return verifyAndAddMachOObject(std::move(NewMember));
   }
 
+  MembersData Data;
+  const Config &C;
+};
+
 static Expected<SmallVector<Slice, 2>>
 buildSlices(ArrayRef<OwningBinary<Archive>> OutputBinaries) {
   SmallVector<Slice, 2> Slices;
 
   for (const auto &OB : OutputBinaries) {
     const Archive &A = *OB.getBinary();
     Expected<Slice> ArchiveSlice = Slice::create(A, &LLVMCtx);
     if (!ArchiveSlice)
       return ArchiveSlice.takeError();
     Slices.push_back(*ArchiveSlice);
   }
   return Slices;
 }
 
 static Error createStaticLibrary(const Config &C) {
-  MembersPerArchitectureMap NewMembers;
-  std::vector<std::unique_ptr<MemoryBuffer>> FileBuffers;
-  for (StringRef FileName : InputFiles)
-    if (Error E = addMember(NewMembers, FileBuffers, FileName, C))
-      return E;
+  MembersBuilder Builder(C);
+  auto DataOrError = Builder.build();
+  if (auto Error = DataOrError.takeError())
+    return Error;
 
-  if (!ArchType.empty()) {
-    uint64_t ArchCPUID = getCPUID(C.ArchCPUType, C.ArchCPUSubtype);
-    if (NewMembers.find(ArchCPUID) == NewMembers.end())
-      return createStringError(std::errc::invalid_argument,
-                               "no library created (no object files in input "
-                               "files matching -arch_only %s)",
-                               ArchType.c_str());
-  }
+  const auto &NewMembers = DataOrError->MembersPerArchitecture;
 
   if (NewMembers.size() == 1) {
     return writeArchive(OutputFile, NewMembers.begin()->second,
                         /*WriteSymtab=*/true,
                         /*Kind=*/object::Archive::K_DARWIN, C.Deterministic,
                         /*Thin=*/false);
   }