[PATCH] D42762: Rewrite the VS Integration Scripts

[Hans Wennborg via Phabricator via llvm-commits]

hans added inline comments.


================
Comment at: llvm/tools/msbuild/LLVM.Cpp.Common.props:47
+    <CLToolExe>clang-cl.exe</CLToolExe>
+    <CLToolPath>$(LLVMInstallDir)bin</CLToolPath>
+
----------------
zturner wrote:
> hans wrote:
> > 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?
> At the moment, you can do this by changing the registry key.  In subsequent patches I can add a UI setting under C/C++ > General.
Is it hard to add, or can you add it to this patch? Changing the registry isn't very good UI and is also global.


================
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">
----------------
zturner wrote:
> hans wrote:
> > I thought you were going to leave the flag logic out of this patch?
> I thought Nico said to only separate out the option remapping into another patch.  I took all of that out.
This file still attempts to duplicate the logic of what flags clang-cl supports, which are ignored, etc. I'd prefer if we didn't do that.

To repeat my concerns:

1. This list of flags would need to be maintained, and I expect it will always end up being incomplete or inaccurate in some other way. The only way to keep it accurate would be if it's auto-generated from clang-cl's tablegen file. But that ties it to a specific clang-cl version, which is the next problem

2. It ties it to a particular clang-cl version, which goes against the point of decoupling the VS integration from the toolchain. As a concrete example, your file is missing /arch:AVX512 and /arch:AVX512F, so those should be added, but only when the user is using a Clang >= r323433. Of course the user could them manually, but why should some flags be listed and some not? And maybe we could handle this with version logic, but now we're getting into *really* hairy maintenance.


================
Comment at: llvm/tools/msbuild/license.txt:1
+====================
+LLVM Release License
----------------
zturner wrote:
> hans wrote:
> > Can't we use the old file? Checking in another copy seems unfortunate.
> I copied this one from the clang-format vs integration folder.  The one at the top level llvm directory has some extra stuff at the bottom which doesn't apply to this, but I don't know if it would still work just as well even with all that extra stuff in it.
Right, the clang-format one is my fault :-) It's formated to fit better in the vsix installer window. Could we make the build system copy it instead of checking in another copy? If that's not easy, never mind this is not important obviously.


https://reviews.llvm.org/D42762