<div class="gmail_quote">On Mon, Mar 12, 2012 at 2:22 PM, Bill Wendling <span dir="ltr"><<a href="mailto:isanbard@gmail.com">isanbard@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Author: void<br>
Date: Mon Mar 12 16:22:35 2012<br>
New Revision: 152578<br>
<br>
URL: <a href="http://llvm.org/viewvc/llvm-project?rev=152578&view=rev" target="_blank">http://llvm.org/viewvc/llvm-project?rev=152578&view=rev</a><br>
Log:<br>
Have clang pay attention to the LIBRARY_PATH environment variable.<br>
<br>
The LIBRARY_PATH environment variable should be honored by clang. Have the<br>
driver pass the directories to the linker.<br>
<rdar://problem/9743567> and PR10296.<br></blockquote><div><br></div><div>Mostly nits below... Also, I know this isn't new code, just had to grumble when I saw it. ;] Feel free to tell me to sod off and clean it up myself / later.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
Added:<br>
cfe/trunk/test/Driver/linker-opts.c<br>
Modified:<br>
cfe/trunk/lib/Driver/Tools.cpp<br>
<br>
Modified: cfe/trunk/lib/Driver/Tools.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Driver/Tools.cpp?rev=152578&r1=152577&r2=152578&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Driver/Tools.cpp?rev=152578&r1=152577&r2=152578&view=diff</a><br>
==============================================================================<br>
--- cfe/trunk/lib/Driver/Tools.cpp (original)<br>
+++ cfe/trunk/lib/Driver/Tools.cpp Mon Mar 12 16:22:35 2012<br>
@@ -88,6 +88,38 @@<br>
}<br>
}<br>
<br>
+static void AddDirectoryList(const ArgList &Args,<br></blockquote><div><br></div><div>Coding conventions like the naming: addDirectory</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+ ArgStringList &CmdArgs,<br>
+ const char *ArgName,<br>
+ const char *DirList) {<br>
+ if (!DirList)<br>
+ return; // Nothing to do.<br></blockquote><div><br></div><div>Rather than encoding this special behavior for a 'const char *' here, what about sinking the getenv call into this routine? I think the abstraction would be better, and it would also isolate the places we reach out to such a nasty interface as getenv.</div>
<div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+<br>
+ StringRef Dirs(DirList);<br>
+ if (Dirs.empty()) // Empty string should not add '.'.<br>
+ return;<br>
+<br>
+ StringRef::size_type Delim;<br>
+ while ((Delim = Dirs.find(llvm::sys::PathSeparator)) != StringRef::npos) {<br>
+ if (Delim == 0) { // Leading colon.<br>
+ CmdArgs.push_back(ArgName);<br>
+ CmdArgs.push_back(".");<br>
+ } else {<br>
+ CmdArgs.push_back(ArgName);<br>
+ CmdArgs.push_back(Args.MakeArgString(Dirs.substr(0, Delim)));<br>
+ }<br>
+ Dirs = Dirs.substr(Delim + 1);<br>
+ }<br></blockquote><div><br></div><div>It would seem much cleaner to use the 'split' methods on StringRef.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+<br>
+ if (Dirs.empty()) { // Trailing colon.<br>
+ CmdArgs.push_back(ArgName);<br>
+ CmdArgs.push_back(".");<br>
+ } else { // Add the last path.<br>
+ CmdArgs.push_back(ArgName);<br>
+ CmdArgs.push_back(Args.MakeArgString(Dirs));<br>
+ }<br>
+}<br>
+<br>
static void AddLinkerInputs(const ToolChain &TC,<br>
const InputInfoList &Inputs, const ArgList &Args,<br>
ArgStringList &CmdArgs) {<br>
@@ -128,6 +160,9 @@<br>
} else<br>
A.renderAsInput(Args, CmdArgs);<br>
}<br>
+<br>
+ // LIBRARY_PATH - included following the user specified library paths.<br>
+ AddDirectoryList(Args, CmdArgs, "-L", ::getenv("LIBRARY_PATH"));<br>
}<br>
<br>
/// \brief Determine whether Objective-C automated reference counting is<br>
@@ -162,38 +197,6 @@<br>
CmdArgs.push_back(Args.MakeArgString(ProfileRT));<br>
}<br>
<br>
-static void AddIncludeDirectoryList(const ArgList &Args,<br>
- ArgStringList &CmdArgs,<br>
- const char *ArgName,<br>
- const char *DirList) {<br>
- if (!DirList)<br>
- return; // Nothing to do.<br>
-<br>
- StringRef Dirs(DirList);<br>
- if (Dirs.empty()) // Empty string should not add '.'.<br>
- return;<br>
-<br>
- StringRef::size_type Delim;<br>
- while ((Delim = Dirs.find(llvm::sys::PathSeparator)) != StringRef::npos) {<br>
- if (Delim == 0) { // Leading colon.<br>
- CmdArgs.push_back(ArgName);<br>
- CmdArgs.push_back(".");<br>
- } else {<br>
- CmdArgs.push_back(ArgName);<br>
- CmdArgs.push_back(Args.MakeArgString(Dirs.substr(0, Delim)));<br>
- }<br>
- Dirs = Dirs.substr(Delim + 1);<br>
- }<br>
-<br>
- if (Dirs.empty()) { // Trailing colon.<br>
- CmdArgs.push_back(ArgName);<br>
- CmdArgs.push_back(".");<br>
- } else { // Add the last path.<br>
- CmdArgs.push_back(ArgName);<br>
- CmdArgs.push_back(Args.MakeArgString(Dirs));<br>
- }<br>
-}<br>
-<br>
void Clang::AddPreprocessingOptions(Compilation &C,<br>
const Driver &D,<br>
const ArgList &Args,<br>
@@ -399,19 +402,19 @@<br>
// frontend into the driver. It will allow deleting 4 otherwise unused flags.<br>
// CPATH - included following the user specified includes (but prior to<br>
// builtin and standard includes).<br>
- AddIncludeDirectoryList(Args, CmdArgs, "-I", ::getenv("CPATH"));<br>
+ AddDirectoryList(Args, CmdArgs, "-I", ::getenv("CPATH"));<br>
// C_INCLUDE_PATH - system includes enabled when compiling C.<br>
- AddIncludeDirectoryList(Args, CmdArgs, "-c-isystem",<br>
- ::getenv("C_INCLUDE_PATH"));<br>
+ AddDirectoryList(Args, CmdArgs, "-c-isystem",<br>
+ ::getenv("C_INCLUDE_PATH"));<br>
// CPLUS_INCLUDE_PATH - system includes enabled when compiling C++.<br>
- AddIncludeDirectoryList(Args, CmdArgs, "-cxx-isystem",<br>
- ::getenv("CPLUS_INCLUDE_PATH"));<br>
+ AddDirectoryList(Args, CmdArgs, "-cxx-isystem",<br>
+ ::getenv("CPLUS_INCLUDE_PATH"));<br>
// OBJC_INCLUDE_PATH - system includes enabled when compiling ObjC.<br>
- AddIncludeDirectoryList(Args, CmdArgs, "-objc-isystem",<br>
- ::getenv("OBJC_INCLUDE_PATH"));<br>
+ AddDirectoryList(Args, CmdArgs, "-objc-isystem",<br>
+ ::getenv("OBJC_INCLUDE_PATH"));<br>
// OBJCPLUS_INCLUDE_PATH - system includes enabled when compiling ObjC++.<br>
- AddIncludeDirectoryList(Args, CmdArgs, "-objcxx-isystem",<br>
- ::getenv("OBJCPLUS_INCLUDE_PATH"));<br>
+ AddDirectoryList(Args, CmdArgs, "-objcxx-isystem",<br>
+ ::getenv("OBJCPLUS_INCLUDE_PATH"));<br>
<br>
// Add C++ include arguments, if needed.<br>
if (types::isCXX(Inputs[0].getType()))<br>
<br>
Added: cfe/trunk/test/Driver/linker-opts.c<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Driver/linker-opts.c?rev=152578&view=auto" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Driver/linker-opts.c?rev=152578&view=auto</a><br>
==============================================================================<br>
--- cfe/trunk/test/Driver/linker-opts.c (added)<br>
+++ cfe/trunk/test/Driver/linker-opts.c Mon Mar 12 16:22:35 2012<br>
@@ -0,0 +1,2 @@<br>
+// RUN: env LIBRARY_PATH=%T/test1 %clang -x c %s -### -o foo 2> %t.log<br>
+// RUN: grep '".*ld.*" .*"-L" "%T/test1"' %t.log</blockquote><div><br></div><div>Do we have no testing for CPATH and co.? If not, we should make a bigger test for all the environment variable based switches (even if the rest are left as FIXMEs). If we do, I'd rather merge this into that test.</div>
<div><br></div><div>Do you need to blacklist this on windows? how is 'env' going to work?</div><div><br></div><div>Finally, can you use FileCheck instead of grep like the other linker option tests? <br></div></div>