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

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson added a comment.

I think this looks pretty good now. Just some minor suggestions for documentation comments etc.



================
Comment at: lld/COFF/Config.h:223
   std::pair<llvm::StringRef, llvm::StringRef> thinLTOPrefixReplace;
+  llvm::StringRef thinLTOIndexFileObjectPrefixReplace;
 
----------------
Can you document this here and in the other Config.h files, specifically noting its relationship with thinLTOPrefixReplace? It's maybe a little confusing that we are replacing the first string in the pair of thinLTOPrefixReplace with this string, so I think it needs to be documented.


================
Comment at: lld/ELF/Driver.cpp:1023
+static std::pair<std::pair<StringRef, StringRef>, StringRef>
+getOldNewOptionsExtra(opt::InputArgList &args, unsigned id) {
+  std::pair<StringRef, StringRef> ret = getOldNewOptions(args, id);
----------------
Add a brief documentation of what this is doing. Ditto for implementations in other Driver.cpp files.


================
Comment at: llvm/include/llvm/LTO/LTO.h:224
+/// nullptr and FinalObjectPrefix is not empty then it replaces the prefix of
+/// the objects for FinalObjectPrefix instead of NewPrefix. OnWrite is
+/// callback which receives module identifier and notifies LTO user that index
----------------
nit: s/for/with


================
Comment at: llvm/include/llvm/LTO/LTO.h:229
                                           std::string NewPrefix,
+                                          std::string NewLinkObjectPrefix,
                                           bool ShouldEmitImportsFiles,
----------------
itf wrote:
> tejohnson wrote:
> > MaskRay wrote:
> > > `NewLinkObjectPrefix` refers to the ELF relocatable object file path. Is there a better parameter name to reflect that than `NewLinkObjectPrefix`? @tejohnson 
> > Maybe FinalObjectPrefix ?
> Would FinalObjectPrefix not be confused with being the prefix of the final executable/lib created at the end of the final linking step, where are these "link objects" get linked together?
Hmm, I think I agree with you. How about NativeObjectPrefix ? Sorry for the renaming churn.


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

https://reviews.llvm.org/D144596