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

[Roger Kim via Phabricator via llvm-commits]

Roger added inline comments.


================
Comment at: llvm/tools/llvm-libtool-darwin/llvm-libtool-darwin.cpp:242-245
+    Expected<NewArchiveMember> NMOrErr =
+        NewArchiveMember::getFile(FileName, C.Deterministic);
+    if (!NMOrErr)
+      return createFileError(FileName, NMOrErr.takeError());
----------------
alexander-shaposhnikov wrote:
> Roger wrote:
> > jhenderson wrote:
> > > 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).
> > I made the suggested changes in D113215 so that we can maintain the changes in this diff as a "moved from" change.
> 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. 
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 :).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113127