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


================
Comment at: llvm/tools/llvm-libtool-darwin/llvm-libtool-darwin.cpp:264-266
+    if (Error E = verifyAndAddMachOObject(std::move(*NMOrErr)))
+      return E;
     return Error::success();
----------------
jhenderson wrote:
> You could simplify this to the inline edit.
I made the suggested changes in D113301 to maintain this change as a whitespace change.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113127