[PATCH] D144596: [lld] Support separate native object file path in --thinlto-prefix-replace

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added a comment.

Mostly looks good. I am on a trip til April 10, but I'll be reasonably responsible reading updates to this patch in the evening.



================
Comment at: lld/ELF/Driver.cpp:1027
+  std::pair<StringRef, StringRef> ret = getOldNewOptions(args, id);
+  std::pair<StringRef, StringRef> newOptions = ret.second.split(';');
+  return {ret.first, newOptions.first, newOptions.second};
----------------
Use C++17  structured binding


================
Comment at: lld/test/COFF/thinlto-index-file-object-prefix-replace.ll:39
+
+
+target datalayout = "e-m:w-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
----------------
delete


================
Comment at: lld/test/ELF/lto/thinlto-index-file-object-prefix-replace.ll:15
+; CHECK-NO-REPLACE: old/subdir/1.o
+; CHECK-NO-REPLACE: old/subdir/2.o
+; CHECK-NO-REPLACE: old/3.o
----------------
Here and throughout, add `-NEXT` if applicable




================
Comment at: lld/test/ELF/lto/thinlto-index-file-object-prefix-replace.ll:39
+
+; Ensure that lld generates error if prefix replace option have 'old;new;obj' format but index file is not set. Ensure that the error is about thinlto-prefix-replace.
+; RUN: not ld.lld --thinlto-prefix-replace="old/;new/;obj/" -shared old/subdir/1.o old/subdir/2.o old/3.o -o /dev/null 2>&1 | FileCheck %s --check-prefix=ERR
----------------
`;; Ensure that lld generates error` can be simplified to `Create an error` since the subject (lld) is implied by the context.


================
Comment at: lld/test/ELF/lto/thinlto-index-file-object-prefix-replace.ll:41
+; RUN: not ld.lld --thinlto-prefix-replace="old/;new/;obj/" -shared old/subdir/1.o old/subdir/2.o old/3.o -o /dev/null 2>&1 | FileCheck %s --check-prefix=ERR
+; ERR: thinlto-prefix-replace
+
----------------
Here and throughout, always include `error:` for an error message, and try including the input filename.



================
Comment at: lld/test/MachO/thinlto-index-file-object-prefix-replace.ll:2
+; REQUIRES: x86
+; RUN: rm -rf %t ; split-file %s %t
+; RUN: mkdir -p %t/old/subdir
----------------
Prefer `&&` to `;` (if `%t` is somehow non-removable, bail out immediately)


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

https://reviews.llvm.org/D144596