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

[Alexander Shaposhnikov via Phabricator via llvm-commits]

alexander-shaposhnikov 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());
----------------
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. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113127