[PATCH] D103985: [lld/mac] Print dylib search details with --print-dylib-search or RC_TRACE_DYLIB_SEARCHING

[Alexander Shaposhnikov via Phabricator via llvm-commits]

alexshap added inline comments.


================
Comment at: lld/MachO/DriverUtils.cpp:179
 
-Optional<std::string> macho::resolveDylibPath(StringRef path) {
-  // TODO: if a tbd and dylib are both present, we should check to make sure
-  // they are consistent.
-  if (fs::exists(path))
-    return std::string(path);
-  else
+static void searchedDylib(const Twine &path, bool found) {
+  if (config->printDylibSearch)
----------------
thakis wrote:
> alexshap wrote:
> > perhaps, logDylibSearchResult would be a better name ?
> > also - maybe use StringRef instead of Twine ?
> The last caller in this file passes a Twine, so StringRef doesn't work.
> 
> I like the shorter name.
I think readability of the code matters, not sure if "searchedDylib" is a sufficiently descriptive name


================
Comment at: lld/MachO/DriverUtils.cpp:192
+  if (dylibExists)
+    return std::string(dylibPath);
+
----------------
thakis wrote:
> alexshap wrote:
> > dylibPath.str() ?
> This is just moving code around that was here previously. `std::string(...)` is how you convert a string view to a string in c++17, so I suppose this is consistent with that.
quick grepping shows some minor inconsistency even inside lld/MachO.  It doesn't change the "functional" side of the problem, but having a consistent code would definitely be a plus. I'm not against any of these methods, but wanted to point this out / so that at least the new code is written in a more consistent way.


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

https://reviews.llvm.org/D103985