[cfe-commits] Win32 default include path improvement

[Daniel Dunbar]

Hi John,

Some comments:
--
> Index: lib/Frontend/InitHeaderSearch.cpp
> ===================================================================
> --- lib/Frontend/InitHeaderSearch.cpp	(revision 79306)
> +++ lib/Frontend/InitHeaderSearch.cpp	(working copy)
> @@ -107,18 +107,60 @@
>    // FIXME: get these from the target?
>
>  #ifdef LLVM_ON_WIN32
> -  if (Lang.CPlusPlus) {
> -    // Mingw32 GCC version 4
> -    AddPath("c:/mingw/lib/gcc/mingw32/4.3.0/include/c++",
> -            System, true, false, false);
> -    AddPath("c:/mingw/lib/gcc/mingw32/4.3.0/include/c++/mingw32",
> -            System, true, false, false);
> -    AddPath("c:/mingw/lib/gcc/mingw32/4.3.0/include/c++/backward",
> -            System, true, false, false);
> +
> +#if defined(_MSC_VER)
> +  const char* vs90comntools = getenv("VS90COMNTOOLS");
> +  const char* vs80comntools = getenv("VS80COMNTOOLS");
> +  const char* vscomntools = NULL;
> +    // If we have both vc80 and vc90, pick version we were compiled with.
> +  if (vs90comntools && vs80comntools) {
> +    #if (_MSC_VER >= 1500)  // VC90
> +        vscomntools = vs90comntools;
> +    #elif (_MSC_VER == 1400) // VC80
> +        vscomntools = vs80comntools;
> +    #else
> +        vscomntools = vs90comntools;
> +    #endif

I think this is overkill. Ultimately this code *really* needs to be a
configuration option, and I don't like guessing things based on the
host compiler.

>    }
> +  else if (vs90comntools)
> +    vscomntools = vs90comntools;
> +  else if (vs80comntools)
> +    vscomntools = vs80comntools;

This would be cleaner if split into a separate function, then the next
line would just be
--
if (const char *VSToolsDir = getVSCommonToolsDir()) {
...
--
or so...

> +  if (vscomntools) {
> +    char path[512];
> +    const char* ep = strstr(vscomntools, "Common7\\Tools");
> +    if (!ep)
> +      ep = vscomntools + strlen(vscomntools);
> +    size_t len = ep - vscomntools;
> +    if ((len + 12) <= sizeof(path)) {
> +      memcpy(path, vscomntools, len);
> +      if ((path[len - 1] != '\\') && (path[len - 1] != '/'))
> +        path[len++] = '\\';
> +      strcpy(path + len, "VC\\include");
> +      AddPath(path, System, false, false, false);
> +    }
> +    else
> +      fprintf(stderr, "VS80COMNTOOLS or VS90COMNTOOLS environment variable "
> +        "value is too long (%d bytes).\n", len);

This is doing lots of manual string editing which in my personal
experience means it has bugs. :)

I would write this as:
--
if (const char *VSToolsDirPtr = getVSCommonToolsDir()) {
  StringRef VSToolsDir(VSToolsDirPtr);
  if (VSToolsDir.endswith("Common7\\Tools"))
    VSToolsDir = VSToolsDir.slice(0, VSToolsDir.size() -
strlen("Common7\\Tools"));

  std::string VSDir = VSToolsDir;
  if (VSToolsDir.back() != '\\' && VSToolsDir.back() != '/')
    VSDir += '\\';
  VSDir += "VC\\include";
  AddPath(VSDir, System, false, false, false);
}
--
which it is a shorter & easier to read IMHO.
--

Thanks,
 - Daniel


On Mon, Aug 17, 2009 at 8:11 AM, John
Thompson<john.thompson.jtsoftware at gmail.com> wrote:
> Here's what I think is an incremental improvement for Win32 default
> include paths, though it's still not optimal.
>
> It uses a couple environment variables installed by Visual Studio to
> guess at the default include path.  It also tries to accomodate MinGW
> and Cygwin, though it uses hard-coded paths.
>
> Note, this supercedes a version of it I put on cfe-dev last week.  I
> added some buffer overrun protection and revised the comments.
>
> -John
>
> --
> John Thompson
> John.Thompson.JTSoftware at gmail.com
>
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>
>