[PATCH] D127246: [LinkerWrapper] Rework the linker wrapper and use owning binaries

[Jon Chesterfield via Phabricator via cfe-commits]

JonChesterfield added a comment.

I've read it but can't promise it's correct -  the diff is large and has some spurious noise in it which distracts significantly from the functional changes.

Would you be willing to split this into two patches, one which renames variables and moves blocks of code around without changing them and a second that has the functional changes?



================
Comment at: clang/tools/clang-linker-wrapper/ClangLinkerWrapper.cpp:338
+        Contents.getBufferIdentifier());
+    auto NewBinaryOrErr = OffloadBinary::create(*BufferCopy);
+    if (!NewBinaryOrErr)
----------------
This is syntactically a bit weird: `std::move(*NewBinaryOrErr)`

Could you spell out the type? I think based on the previous version it's probably an Expected<unique_ptr<>> but my first thought was whether a unique_ptr was being heap allocated by ::create


================
Comment at: clang/tools/clang-linker-wrapper/ClangLinkerWrapper.cpp:784
     };
-    Conf.PostInternalizeModuleHook = [&](size_t, const Module &M) {
+    Conf.PostInternalizeModuleHook = [&, Arch](size_t, const Module &M) {
       SmallString<128> TempFile;
----------------
Why call out Arch here? Passing a StringRef by reference via the & should be fine


================
Comment at: clang/tools/clang-linker-wrapper/ClangLinkerWrapper.cpp:903
 
   // We assume visibility of the whole program if every input file was bitcode.
+  bool WholeProgram = InputFiles.empty();
----------------
Existing comment I see, but what justifies that assumption?


================
Comment at: clang/tools/clang-linker-wrapper/ClangLinkerWrapper.cpp:1136
+
+  Triple TheTriple = Triple(Images.front().StringData.lookup("triple"));
+  auto FileOrErr = nvptx::fatbinary(InputFiles, TheTriple);
----------------
This is close but not quite a copy of existing code, moving it around in the file at the same time as changing it makes the diff significantly harder to follow


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127246