Hahnfeld added a comment.

In https://reviews.llvm.org/D43197#1027139, @gtbercea wrote:

> In https://reviews.llvm.org/D43197#1011256, @Hahnfeld wrote:
>
> > Looking more closely at the patch, this doesn't seem to look into the `lib` 
> > / `lib64` next to the compiler. I'm not sure if `LIBRARY_PATH` is set for 
> > every installation, so I think we should add this one to catch the obvious 
> > case. This probably needs some attention for the tests because they'll find 
> > the just-built libraries...
>
>
> The contract with the user us that the .bc lib needs to be in LIBRARY_PATH, 
> this is what we require today. Inlining of the runtime is not essential for 
> correctness.


And I don't think this is clever. The compiler should always try to 
automatically find the files it needs and looking in the lib directory next to 
the compiler installation isn't hard in that case and a guess that will meet 
99% of deployments.



================
Comment at: lib/Driver/ToolChains/Cuda.cpp:536-542
+      StringRef CompilerPath = env;
+      while (!CompilerPath.empty()) {
+        std::pair<StringRef, StringRef> Split =
+            CompilerPath.split(llvm::sys::EnvPathSeparator);
+        LibraryPaths.push_back(Split.first);
+        CompilerPath = Split.second;
+      }
----------------
gtbercea wrote:
> Hahnfeld wrote:
> > `tools::addDirectoryList` uses `StringRef::find`, I'm not sure if 
> > `StringRef::split` creates real copies of the string...
> What is your suggestion?
IMO you should use whatever existing code does, in that case `StringRef::find`.


Repository:
  rC Clang

https://reviews.llvm.org/D43197



_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to