[PATCH] D42326: [COFF] don't replace import library if contents are unchanged

[Rui Ueyama via Phabricator via llvm-commits]

ruiu added inline comments.


================
Comment at: lld/COFF/Driver.cpp:544-545
+  // If the import library already exists, replace it only if the contents
+  // have changed.
+  llvm::ErrorOr<std::unique_ptr<MemoryBuffer>> OldBuf =
+      MemoryBuffer::getFile(Path);
----------------
Omit `llvm::`.


================
Comment at: lld/COFF/Driver.cpp:555
+                                     false)) {
+      HandleError(std::move(E));
+    } else {
----------------
I'd do early return here.


================
Comment at: lld/COFF/Driver.cpp:562
+          HandleError(errorCodeToError(EC));
+      }
+    }
----------------
Also here.


================
Comment at: lld/COFF/Driver.cpp:565-567
+    Error E =
+        writeImportLibrary(LibName, Path, Exports, Config->Machine, false);
+    HandleError(std::move(E));
----------------
Isn't `HandleError(writeImportLibrary(...))` better?


================
Comment at: lld/COFF/Driver.cpp:565-567
+    Error E =
+        writeImportLibrary(LibName, Path, Exports, Config->Machine, false);
+    HandleError(std::move(E));
----------------
ruiu wrote:
> Isn't `HandleError(writeImportLibrary(...))` better?
Since "then" part is much longer than this "else" part, I'd flip the condition and early return. I.e.

  if (!OldBuf) {
    HandleError(writeImportLibrary(...));
    return;
  }

  // Replace the existing file only when the new file is different.


https://reviews.llvm.org/D42326