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

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added a comment.

Thanks for the patch! I have gone through the code and it looks in a good shape. Left some suggestions.



================
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());
----------------
Hopefully share with other template instantiations to decrease the executable size.


================
Comment at: lld/Common/DriversMain.cpp:152
+namespace lld {
+bool inTestOutputDisabled = false;
+}
----------------



================
Comment at: lld/Common/DriversMain.cpp:178
+namespace lld {
+Result lldMain(llvm::ArrayRef<const char *> args, llvm::raw_ostream &stdoutOS,
+               llvm::raw_ostream &stderrOS, llvm::ArrayRef<DriverDef> drivers,
----------------
`lld::lldMain` (see https://llvm.org/docs/CodingStandards.html#use-namespace-qualifiers-to-implement-previously-declared-functions)

Fix other places as well.


================
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) {
----------------
https://llvm.org/docs/CodingStandards.html#use-namespace-qualifiers-to-implement-previously-declared-functions


================
Comment at: lld/ELF/Options.td:739
+// Flag that selects the driver, it should be ignored.
+def flavor : Separate<["-"], "flavor">;
----------------
We should avoid ignoring `-flavor` in all drivers. It's probably not difficult to strip `-flavor` from the driver dispatcher.


================
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) {
----------------
https://llvm.org/docs/CodingStandards.html#use-namespace-qualifiers-to-implement-previously-declared-functions


================
Comment at: lld/unittests/AsLibAll/AllDrivers.cpp:18
+
+bool lldInvoke(std::vector<const char *> args) {
+  args.push_back("--version");
----------------
`static`


================
Comment at: lld/unittests/AsLibAll/CMakeLists.txt:1
+add_lld_unittests(LLDAsLibAllTests
+  AllDrivers.cpp
----------------
Add a file-level comment explaining what `AsLibAll` means.


================
Comment at: lld/unittests/AsLibELF/CMakeLists.txt:1
+add_lld_unittests(LLDAsLibELFTests
+  ROCm.cpp
----------------
Add a file-level comment explaining what `AsLibELF` means.


================
Comment at: lld/unittests/AsLibELF/ROCm.cpp:8
+//===----------------------------------------------------------------------===//
+
+#include "lld/Common/Driver.h"
----------------
Add a comment about the purpose.


================
Comment at: lld/unittests/AsLibELF/ROCm.cpp:21
+  if (llvm::StringRef(path).contains("%")) {
+    expanded.replace(expanded.find("%S"), 2, thisFile.data(), thisFile.size());
+  }
----------------
remove braces


================
Comment at: lld/unittests/AsLibELF/ROCm.cpp:28
+
+bool lldInvoke(const char *inPath, const char *outPath) {
+  std::vector<const char *> args{"ld.lld", "-shared", inPath, "-o", outPath};
----------------
`static`


================
Comment at: lld/unittests/AsLibELF/SomeDrivers.cpp:12
+
+LLD_HAS_DRIVER(elf);
+
----------------
`static`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119049