[cfe-commits] Win32 default include path improvement

[John Thompson]

Daniel,

Sorry, I didn't see this before replying to the cfe-dev message.  I've
enclosed an alternative patch with some of your changes.

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

I see.  But for now, since the tests need to find the standard
includes without having to be told where, I'm hoping you can let this
slide for now as a temporary hack, unless you have a better solution.
(Maybe pass in additional options via an enviroment variable?  But
note the problem with TestRunner.py about the environment I mention
below...)  Unless you really want me to set up a g: drive in order to
run the tests...

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

Yeah, this is probably more appropriate here using the string objects,
as it better fits with the Clang programming style, and I need to get
familiar with the ADT stuff.  However, when I see code using string
objects, my first thought concerns how many allocations and
deallocations are being done under the covers (two each in this case),
and how seemingly small code translates into many times the number of
instructions executed.  But in this case, neither of these things
matter, so the fewer lines and more readable option is probably
better.  One thing I noticed is that using "endswith" means the path
has to end with a separator, but since that's what the VS installer
puts in, it's probably fine.

Note that I left in the hard-coded paths if the environment variable
is not found because of the issue with the TestRunner.py script losing
those envirnment variables.  It looks like maybe the script needs to
explicitly pass them through, but since I don't know Python yet, I
haven't tried to figure it out.  If you have some spare cycles some
time along with your other work on the script...

-John

-- 
John Thompson
John.Thompson.JTSoftware at gmail.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: win32_default_include_path.patch
Type: application/octet-stream
Size: 3325 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20090818/8ca992fe/attachment.obj>