[PATCH] D42762: Rewrite the VS Integration Scripts

[Hans Wennborg via Phabricator via llvm-commits]

hans added inline comments.


================
Comment at: llvm/CMakeLists.txt:234
-  set(CPACK_NSIS_EXTRA_UNINSTALL_COMMANDS
-    "ExecWait '$INSTDIR/tools/msbuild/uninstall.bat'")
   if( CMAKE_CL_64 )
----------------
Now that the LLVM toolchain installer no longer includes the VS integration, is there some way we could point the user to the marketplace to install the integration, or could we include the vsix in the installer and launch it for them?


================
Comment at: llvm/tools/msbuild/LLVM.Cpp.Common.props:1
+<Project xmlns="http://schemas.microsoft.com/developer/msbuild/2003">
+  <!-- The general order of executing an MSBuild file is roughly:
----------------
Do we know what other versions this works with besides VS 2017?


================
Comment at: llvm/tools/msbuild/LLVM.Cpp.Common.props:47
+    <CLToolExe>clang-cl.exe</CLToolExe>
+    <CLToolPath>$(LLVMInstallDir)bin</CLToolPath>
+
----------------
Is there a way for the user to point to another LLVM installation? This was one of the reasons for separating the toolset from the toolchain, right?


================
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">
----------------
I thought you were going to leave the flag logic out of this patch?


================
Comment at: llvm/tools/msbuild/install.bat:1
 @echo off
 
----------------
I thought you meant to delete this file? I can see that it's still useful for development, but maybe then we should rename it or put a comment on the top or something explaining what it does, since its purpose is now changing from what it was before.


================
Comment at: llvm/tools/msbuild/license.txt:1
+====================
+LLVM Release License
----------------
Can't we use the old file? Checking in another copy seems unfortunate.


================
Comment at: llvm/tools/msbuild/llvm.sln:1
+
+Microsoft Visual Studio Solution File, Format Version 12.00
----------------
Should we have a cmake build target to build this vsix, similar to what we do for the clang-format one, which gets built by the "clang_format_vsix" target?


https://reviews.llvm.org/D42762