[PATCH] D42762: Rewrite the VS Integration Scripts

Thu Feb 1 02:58:53 PST 2018

hans added inline comments.

================
Comment at: llvm/tools/msbuild/Clang.Cpp.Common.props:41
+    
+    <!-- The registry key may not be set if it's an old installer, try the newest version that exists -->
+    <LLVMVersion Condition="'$(LLVMVersion)' == '' and Exists('$(LLVMInstallDir)\lib\clang\7.0.0')">7.0.0</LLVMVersion>
----------------
As I mentioned before, separating the toolset config from the actual toolchain installation makes me a little nervous.

But if we're doing it, the version checks below should probably include the .1 versions too, i.e. at least 5.0.1 and 6.0.1.

================
Comment at: llvm/tools/msbuild/Clang.Cpp.Common.props:49
+    <CLToolExe>clang-cl.exe</CLToolExe>
+    <CLToolPath>$(LLVMInstallDir)bin</CLToolPath>
+
----------------
It's embarrassing that we didn't figure this out before :-)

With this, we can remove

```
 if (MSVC)
    list(APPEND CLANG_LINKS_TO_CREATE ../msbuild-bin/cl)
  endif()
```

from Clang's tools/driver/CMakeLists.txt

================
Comment at: llvm/tools/msbuild/LLVM.props:8
+    <!-- Friendly names added to the PlatformToolset in the property pages. -->
+    <_PlatformToolsetFriendlyNameFor_llvm Condition="'$(_PlatformToolsetFriendlyNameFor_llvm)' == ''">Clang for Windows</_PlatformToolsetFriendlyNameFor_llvm>
+  </PropertyGroup>
----------------
Hmm, we previously intentionally called the toolset "LLVM" with the thinking that it would eventually include lld and designated the complete llvm toolchain, not just Clang. And is the "for Windows" part necessary?

================
Comment at: llvm/tools/msbuild/Toolset.targets:38
+    
+    <!-- Warn if Fiber Safe Optimizations are enabled, and then ignore them. -->
+    <Warning Condition="'%(ClCompile.EnableFiberSafeOptimizations)' == 'true'"
----------------
This seems to duplicate a lot of logic from clang-cl. It's nice to provide a good UI for the user, but maintaining this seems a lot of work. Are you not concerned that this will rot?

================
Comment at: llvm/tools/msbuild/Toolset.targets:46
+             File="@(ClCompile)(0,0)"
+             Text="clang-cl does not support MSVC Link Time Optimization.  Disable this option in compatibility settings to silence this warning."/>
+
----------------
But maybe we want clang-cl to map this to -flto one day. Now we need to update two places. And with the toolset/toolchain install split, the two places may be installed separately :-/

================
Comment at: llvm/tools/msbuild/Toolset.targets:73
+
+    <!-- Warn if /Zc:wchar_t- is specified, and then ignore it. -->
+    <Warning Condition="'%(ClCompile.ForceConformanceInForLoopScope)' == 'false'"
----------------
Comment copy-paste-o?

================
Comment at: llvm/tools/msbuild/Toolset.targets:83
+    
+    <!-- Warn if XML Documentation is generated, and then ignore it. -->
+    <Warning Condition="'%(ClCompile.GenerateXMLDocumentationFiles)' == 'true'"
----------------
Keeping up with all these flags seems like a huge amount of work. Why not just let clang-cl ignore it?

================
Comment at: llvm/tools/msbuild/Toolset.targets:111
+           File="@(ClCompile)(0,0)"
+           Text="clang-cl does not support OpenMP (/openmp).  This file cannot be compiled."/>
+    
----------------
Maybe it will one day. The OpenMP folks claim their runtime works on Windows and we've shipped it for some releases. It seems better to keep this logic tied to the toolchain?

================
Comment at: llvm/tools/msbuild/Toolset.targets:128
+             -->
+        <WholeProgramOptimization/>
+        <EnableFiberSafeOptimizations/>
----------------
More stuff to keep in sync with MSVC's options and clang-cl :-/

================
Comment at: llvm/tools/msbuild/Toolset.targets:166
+       instead.  The code below is mostly a straight fork from the common Microsoft build file,
+       with only cl.xml replaced with clang-cl.xml. -->
+  <PropertyGroup>
----------------
I guess we'll need to update the fork with each VS release (but still stay compatible with old versions?)

================
Comment at: llvm/tools/msbuild/clang-cl.xml:1
+<?xml version="1.0" encoding="utf-8"?>
+<Rule Name="clang-cl" PageTemplate="tool" DisplayName="C/C++" SwitchPrefix="/" Order="10" xmlns="http://schemas.microsoft.com/build/2009/properties" xmlns:x="http://schemas.microsoft.com/winfx/2006/xaml" xmlns:sys="clr-namespace:System;assembly=mscorlib">
----------------
Maybe add a short top-level comment about what this file is.

================
Comment at: llvm/tools/msbuild/clang-cl.xml:343
+  <BoolProperty ReverseSwitch="openmp-" Name="OpenMPSupport" DisplayName="Open MP Support" Description="This switch is incompatible with clang-cl and will generate an error if set to true.  (/openmp)" Category="Incompatible" Switch="openmp" F1Keyword="VC.Project.VCCLCompilerTool.OpenMP">
+  </BoolProperty>
+  <BoolProperty Name="EnableModules" DisplayName="Enable C++ Modules (experimental)" Description="This switch is incompatible with clang-cl and will generate an error if set to true.  (/experimental:module)" Category="Incompatible" Switch="experimental:module" F1Keyword="VC.Project.VCCLCompilerTool.ModulesSupport">
----------------
This seems like a huge amount of stuff to keep in sync with MSVC and clang-cl's flags.

================
Comment at: llvm/tools/msbuild/install.bat:10
+REM Older versions of VS would look for these files in the Program Files\MSBuild directory
+REM but with VS2017 it seems to look for these directly in the Visual Studio instance.
+REM This means we'll need to do a little extra work to properly detect all the various
----------------
Don't we want to support at least 2015 too?

================
Comment at: llvm/tools/msbuild/install.bat:20
+ECHO Installing x64 Platform Toolset
+SET PlatformToolsets=%VCTargets%\Platforms\x64\PlatformToolsets
+IF NOT EXIST "%PlatformToolsets%\llvm" mkdir "%PlatformToolsets%\llvm"
----------------
This should probably have quotes just to be safe.

https://reviews.llvm.org/D42762