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

[Chandler Carruth]

On Mon, Mar 12, 2012 at 2:22 PM, Bill Wendling <isanbard at gmail.com> wrote:

> Author: void
> Date: Mon Mar 12 16:22:35 2012
> New Revision: 152578
>
> URL: http://llvm.org/viewvc/llvm-project?rev=152578&view=rev
> Log:
> Have clang pay attention to the LIBRARY_PATH environment variable.
>
> The LIBRARY_PATH environment variable should be honored by clang. Have the
> driver pass the directories to the linker.
> <rdar://problem/9743567> and PR10296.
>

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.


>
> Added:
>    cfe/trunk/test/Driver/linker-opts.c
> Modified:
>    cfe/trunk/lib/Driver/Tools.cpp
>
> 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


> +                             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.

+
> +  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.


> +
> +  if (Dirs.empty()) { // Trailing colon.
> +    CmdArgs.push_back(ArgName);
> +    CmdArgs.push_back(".");
> +  } else { // Add the last path.
> +    CmdArgs.push_back(ArgName);
> +    CmdArgs.push_back(Args.MakeArgString(Dirs));
> +  }
> +}
> +
>  static void AddLinkerInputs(const ToolChain &TC,
>                             const InputInfoList &Inputs, const ArgList
> &Args,
>                             ArgStringList &CmdArgs) {
> @@ -128,6 +160,9 @@
>     } else
>       A.renderAsInput(Args, CmdArgs);
>   }
> +
> +  // LIBRARY_PATH - included following the user specified library paths.
> +  AddDirectoryList(Args, CmdArgs, "-L", ::getenv("LIBRARY_PATH"));
>  }
>
>  /// \brief Determine whether Objective-C automated reference counting is
> @@ -162,38 +197,6 @@
>   CmdArgs.push_back(Args.MakeArgString(ProfileRT));
>  }
>
> -static void AddIncludeDirectoryList(const ArgList &Args,
> -                                    ArgStringList &CmdArgs,
> -                                    const char *ArgName,
> -                                    const char *DirList) {
> -  if (!DirList)
> -    return; // Nothing to do.
> -
> -  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);
> -  }
> -
> -  if (Dirs.empty()) { // Trailing colon.
> -    CmdArgs.push_back(ArgName);
> -    CmdArgs.push_back(".");
> -  } else { // Add the last path.
> -    CmdArgs.push_back(ArgName);
> -    CmdArgs.push_back(Args.MakeArgString(Dirs));
> -  }
> -}
> -
>  void Clang::AddPreprocessingOptions(Compilation &C,
>                                     const Driver &D,
>                                     const ArgList &Args,
> @@ -399,19 +402,19 @@
>   // frontend into the driver. It will allow deleting 4 otherwise unused
> flags.
>   // CPATH - included following the user specified includes (but prior to
>   // builtin and standard includes).
> -  AddIncludeDirectoryList(Args, CmdArgs, "-I", ::getenv("CPATH"));
> +  AddDirectoryList(Args, CmdArgs, "-I", ::getenv("CPATH"));
>   // C_INCLUDE_PATH - system includes enabled when compiling C.
> -  AddIncludeDirectoryList(Args, CmdArgs, "-c-isystem",
> -                          ::getenv("C_INCLUDE_PATH"));
> +  AddDirectoryList(Args, CmdArgs, "-c-isystem",
> +                   ::getenv("C_INCLUDE_PATH"));
>   // CPLUS_INCLUDE_PATH - system includes enabled when compiling C++.
> -  AddIncludeDirectoryList(Args, CmdArgs, "-cxx-isystem",
> -                          ::getenv("CPLUS_INCLUDE_PATH"));
> +  AddDirectoryList(Args, CmdArgs, "-cxx-isystem",
> +                   ::getenv("CPLUS_INCLUDE_PATH"));
>   // OBJC_INCLUDE_PATH - system includes enabled when compiling ObjC.
> -  AddIncludeDirectoryList(Args, CmdArgs, "-objc-isystem",
> -                          ::getenv("OBJC_INCLUDE_PATH"));
> +  AddDirectoryList(Args, CmdArgs, "-objc-isystem",
> +                   ::getenv("OBJC_INCLUDE_PATH"));
>   // OBJCPLUS_INCLUDE_PATH - system includes enabled when compiling ObjC++.
> -  AddIncludeDirectoryList(Args, CmdArgs, "-objcxx-isystem",
> -                          ::getenv("OBJCPLUS_INCLUDE_PATH"));
> +  AddDirectoryList(Args, CmdArgs, "-objcxx-isystem",
> +                   ::getenv("OBJCPLUS_INCLUDE_PATH"));
>
>   // Add C++ include arguments, if needed.
>   if (types::isCXX(Inputs[0].getType()))
>
> 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.

Do you need to blacklist this on windows? how is 'env' going to work?

Finally, can you use FileCheck instead of grep like the other linker option
tests?
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120312/5b40e4cc/attachment.html>