[cfe-commits] r152578 - in /cfe/trunk: lib/Driver/Tools.cpp test/Driver/linker-opts.c

[Bill Wendling]

On Mar 12, 2012, at 2:50 PM, Chandler Carruth <chandlerc at google.com> 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