[PATCH] D42762: Rewrite the VS Integration Scripts

[Shoaib Meenai via Phabricator via llvm-commits]

smeenai added inline comments.


================
Comment at: llvm/tools/msbuild/LLVM.Cpp.Common.props:6
+       3) │  └─ Import Toolset specific props (e.g. $(VCTargets)Platforms\Win32\PlatformToolsets\llvm\Toolset.props)
+       4) │     └─ Import This File (Clang.Cpp.Common.props)
+       5) │─ Core logic of vcxproj (define files, override properties, etc)
----------------
I think this should say LLVM.Cpp.Common.props instead of Clang.Cpp.Common.props?


================
Comment at: llvm/tools/msbuild/LLVM.Cpp.Common.props:42
+    <LLVMInstallDir Condition="'$(LLVMInstallDir)' == ''">$(Registry:HKEY_LOCAL_MACHINE\SOFTWARE\Wow6432Node\LLVM at LLVM)</LLVMInstallDir>
+    <ClangClExecutable>$(LLVMInstallDir)bin\clang-cl.exe</ClangClExecutable>
+  </PropertyGroup>
----------------
Is there a way to add some sort of error here if LLVMInstallDir is still empty? Is it worth it?


================
Comment at: llvm/tools/msbuild/LLVM.Cpp.Common.props:72
+
+      <AdditionalOptions>-m$(PlatformArchitecture) %(AdditionalOptions)</AdditionalOptions>
+    </ClCompile>
----------------
Not that it really matters right now, but this wouldn't do the right thing for arm or arm64, right?


================
Comment at: llvm/tools/msbuild/LLVM.Cpp.Common.targets:56
+    <!-- Warn if Whole Program Optimization is enabled, and then ignore it. -->
+    <Warning Condition="'%(ClCompile.WholeProgramOptimization)' == 'true'"
+             File="@(ClCompile)(0,0)"
----------------
If users want to use LTO with clang-cl + lld-link, are they just expected to set the appropriate compiler and linker options manually?


================
Comment at: llvm/tools/msbuild/LLVM.Cpp.Common.targets:85
+
+    <!-- Warn if /Zc:wchar_t- is specified, and then ignore it. -->
+    <Warning Condition="'%(ClCompile.ForceConformanceInForLoopScope)' == 'false'"
----------------
This comment is swapped...


================
Comment at: llvm/tools/msbuild/LLVM.Cpp.Common.targets:90
+
+    <!-- Warn if /Zc:forScope- is specified, and then ignore it. -->
+    <Warning Condition="'%(ClCompile.TreatWChar_tAsBuiltInType)' == 'false'"
----------------
...with this.


================
Comment at: llvm/tools/msbuild/LLVM.Cpp.Common.targets:118
+           File="@(ClCompile)(0,0)"
+           Text="clang-cl does not support WinRT (/ZW, /ZW:nostdlib).  This file cannot be compiled."/>
+
----------------
I think it's more accurate to describe this as not supporting C++/CX, since clang does support WinRT using straight up COM.


https://reviews.llvm.org/D42762