[PATCH] D147960: [lld-macho] Cache discovered dylib paths

[Jez Ng via Phabricator via llvm-commits]

int3 added inline comments.


================
Comment at: lld/MachO/DriverUtils.cpp:191-194
+  CachedHashStringRef key(dylibPath);
+  auto entry = resolvedDylibs.find(key);
+  if (entry != resolvedDylibs.end())
+    return entry->second;
----------------
keith wrote:
> int3 wrote:
> > how do you feel about using a similar approach to `loadDylib` below where we take a reference to the map value and then modify it? Aside from saving us two lines of `resolvedDylibs[key] = file`, it also serves as a negative cache -- if the key exists but the value is an empty string, then we can just return `std::nullopt`
> > 
> > (I notice we don't actually do this negative cache checking in `loadDylib`, but I guess in that case it matters less since we don't expect that cache lookup to fail in the common error-free case)
> I think I'm missing the C++-ism for how to do this, in this case the value isn't a pointer to a StringRef, can we still use it in that way or should it be changed to be that? Also right now a missing value / empty string won't ever get cached, do you think we should cache that in the fallthrough case to handle the negative cache case you're talking about?
You don't need a pointer, just a reference :) basically I'm suggesting this (inserted after lines 193-196 in your diff)

```
auto entry = resolvedDylibPaths.find(key);
if (entry != resolvedDylibPaths.end()) {
  if (entry->second.empty())
    return {};
  else
    return entry->second;
}
StringRef &s = resolvedDylibPaths[key]; // creates an empty string value entry for `key`
```

And then below would be

```
return s = saver.save(tbdPath.str());
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147960