[PATCH] D119049: [LLD] Allow usage of LLD as a library

[Alexandre Ganea via Phabricator via llvm-commits]

aganea added a comment.

Thanks for taking a in-depth review @MaskRay, I appreciate!



================
Comment at: lld/Common/DriversMain.cpp:60
+  // to allow detecting the -m argument from arguments in them.
+  SmallVector<const char *, 256> expandedArgs(args.data(),
+                                              args.data() + args.size());
----------------
MaskRay wrote:
> Hopefully share with other template instantiations to decrease the executable size.
I checked with CruncherSharp, there are already `,256` instances in the PE image.{F26537168}


================
Comment at: lld/ELF/Driver.cpp:111
+namespace elf {
+bool link(ArrayRef<const char *> args, llvm::raw_ostream &stdoutOS,
+          llvm::raw_ostream &stderrOS, bool exitEarly, bool disableOutput) {
----------------
MaskRay wrote:
> https://llvm.org/docs/CodingStandards.html#use-namespace-qualifiers-to-implement-previously-declared-functions
This pattern works only if the symbol is "public", as in, declared in a header that has been seen already by the current TU. Which is not the case here, I've removed declarations for direct drivers calls, only `lld::lldMain` remains in Drivers.h. Same comment for the other places where this pattern isn't applicable.


================
Comment at: lld/MachO/Driver.cpp:1367
+namespace macho {
+bool link(ArrayRef<const char *> argsArr, llvm::raw_ostream &stdoutOS,
+          llvm::raw_ostream &stderrOS, bool exitEarly, bool disableOutput) {
----------------
MaskRay wrote:
> https://llvm.org/docs/CodingStandards.html#use-namespace-qualifiers-to-implement-previously-declared-functions
It isn't applicable here, since I remove the declarations from `Driver.h`


================
Comment at: lld/tools/lld/lld.cpp:64
 
+LLD_HAS_DRIVER(coff);
+LLD_HAS_DRIVER(elf);
----------------
MaskRay wrote:
> Latest clang gives warnings in a `-DLLD_DEFAULT_LD_LLD_IS_MINGW=on` build.
> ```
> /home/maskray/llvm/lld/tools/lld/lld.cpp:64:21: warning: extra ';' outside of a function is incompatible with C++98 [-Wc++98-compat-extra-semi]
> LLD_HAS_DRIVER(coff);
>                     ^
> ```
Hmm I don't see this with latest patch, would you have a chance to check again if it's still there?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119049