On Mar 12, 2012, at 2:50 PM, Chandler Carruth <[email protected]> wrote:
> 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. > > > Modified: cfe/trunk/lib/Driver/Tools.cpp > URL: > http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Driver/Tools.cpp?rev=152578&r1=152577&r2=152578&view=diff > ============================================================================== > --- cfe/trunk/lib/Driver/Tools.cpp (original) > +++ cfe/trunk/lib/Driver/Tools.cpp Mon Mar 12 16:22:35 2012 > @@ -88,6 +88,38 @@ > } > } > > +static void AddDirectoryList(const ArgList &Args, > > Coding conventions like the naming: addDirectory Okay. > + ArgStringList &CmdArgs, > + const char *ArgName, > + const char *DirList) { > + if (!DirList) > + return; // Nothing to do. > > 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. > That doesn't sound like a horrible idea. :) > + > + StringRef Dirs(DirList); > + if (Dirs.empty()) // Empty string should not add '.'. > + return; > + > + StringRef::size_type Delim; > + while ((Delim = Dirs.find(llvm::sys::PathSeparator)) != StringRef::npos) { > + if (Delim == 0) { // Leading colon. > + CmdArgs.push_back(ArgName); > + CmdArgs.push_back("."); > + } else { > + CmdArgs.push_back(ArgName); > + CmdArgs.push_back(Args.MakeArgString(Dirs.substr(0, Delim))); > + } > + Dirs = Dirs.substr(Delim + 1); > + } > > It would seem much cleaner to use the 'split' methods on StringRef. > > + > Added: cfe/trunk/test/Driver/linker-opts.c > URL: > http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Driver/linker-opts.c?rev=152578&view=auto > ============================================================================== > --- cfe/trunk/test/Driver/linker-opts.c (added) > +++ cfe/trunk/test/Driver/linker-opts.c Mon Mar 12 16:22:35 2012 > @@ -0,0 +1,2 @@ > +// RUN: env LIBRARY_PATH=%T/test1 %clang -x c %s -### -o foo 2> %t.log > +// RUN: grep '".*ld.*" .*"-L" "%T/test1"' %t.log > > 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. > There is a testcase for it (Ben added it with his previous changes). I can merge the two. > Do you need to blacklist this on windows? how is 'env' going to work? > Maybe? > Finally, can you use FileCheck instead of grep like the other linker option > tests? Never!!! :-) I was actually going off of one of the other linker tests. The one checking for '-###' output. But I'll see what I can do when I merge this with the other test. -bw _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
