[PATCH] D98559: [lld-macho] Implement -dependency_info

[Jez Ng via Phabricator via llvm-commits]

int3 accepted this revision.
int3 added inline comments.
This revision is now accepted and ready to land.


================
Comment at: lld/MachO/Driver.cpp:57-58
 
 Configuration *lld::macho::config;
+DependencyTracker *lld::macho::depTracker;
 
----------------
I think we could drop `lld::` from both of these


================
Comment at: lld/MachO/Driver.cpp:821-831
+  auto findDepInfoPath = [](const Arg *arg) -> StringRef {
+    if (!arg)
+      return "";
+    StringRef path = arg->getValue();
+    if ((fs::exists(path) && !fs::can_write(path))) {
+      warn("Ignoring dependency_info option since specified path is not "
+           "writeable.");
----------------
I don't see why this needs to be in a lambda vs just putting it in the constructor of DependencyTracker (and moving the definition of the ctor into DriverUtils.cpp).

Also, `args.getLastArgValue(OPT_dependency_info)` will return an empty string if the option is not available, so you can skip the null check


================
Comment at: lld/MachO/Driver.h:91-93
+    // Denotes the files that do not exist(?)
+    // FIXME: This one is weird. I(vyng) take it to mean all the files
+    // that the linker attempted to look at but which did not exist.
----------------
re the "weird" comment: I think the purpose of this is so that the build system can re-invoke the linker when any of its input files change (including input files that did not previously exist)


================
Comment at: lld/MachO/Driver.h:106-108
+
+  // This is based only on observations. There is no real
+  // documentation on what the opcodes could be.
----------------
rm?


================
Comment at: lld/MachO/Driver.h:93
+  // constructed.
+  std::set<std::string> notFounds;
+
----------------
oontvoo wrote:
> int3 wrote:
> > LLVM tries to avoid `std::set` (and `std::map`): https://llvm.org/docs/ProgrammersManual.html#set-like-containers-std-set-smallset-setvector-etc
> > 
> > In this case we could use a `DenseSet`
> I've thought about this, and IIRC DenseSet's doesn't offer the ordering we need.
> And I'd rather not have to dump it to a vector and re-sort at the end.
> Is there other  ds we could use?
ah I see. yeah I guess std::set is good then


================
Comment at: lld/MachO/DriverUtils.cpp:253
+
+inline void macho::DependencyTracker::logFileNotFound(Twine path) {
+  if (active)
----------------
I think we're suppose to pass Twines as `const` references rather than by value, so that the temporary values they reference don't get freed


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98559