[PATCH] D113063: [lld-macho] Cache discovered framework paths

[Jez Ng via Phabricator via llvm-commits]

int3 accepted this revision.
int3 added a comment.
This revision is now accepted and ready to land.

Thanks!



================
Comment at: lld/MachO/Driver.cpp:125-126
+    if (Optional<StringRef> path = resolveDylibPath(symlink.str())) {
+      resolvedFrameworks[key] = *path;
       return path;
+    }
----------------
ditto, we could return the assignment value and do away with the braces


================
Comment at: lld/MachO/Driver.cpp:117-119
+          auto saved = saver.save(suffixed.str());
+          resolvedPaths[key] = saved;
+          return saved;
----------------
oontvoo wrote:
> keith wrote:
> > int3 wrote:
> > > code style is to use explicit type names instead of `auto` unless the type is something complicated like an iterator
> > > 
> > > also I would personally prefer `return resolvedPaths[key] = second;`, but up to you
> > Thanks for the feedback! Updated accordingly.
> > 
> > Is it possible for us to setup some automation here to flag these?
> > Is it possible for us to setup some automation here to flag these?
> 
> I suspect it's hard because the rules can be ambiguous and sometimes opinion-based :) 
> The usual "rules of thumb" I'd use is that if it's hard to tell at the callsite what the type is, then it should be spelled out. (even then there're exceptions like the iterator's types and lambda, etc)
yeah I'm not aware of a way to lint for this...

thanks for shortening the return + assignment! I was actually thinking we could go all the way and do away with `saved` too, i.e. just `return resolvedFrameworks[key] = saver.save(suffixed.str());`... then the `if` wouldn't need braces


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113063