[PATCH] D30758: Update clang-cl driver for MSVC 2017

[Zachary Turner via Phabricator via llvm-commits]

zturner added inline comments.


================
Comment at: clang/lib/Driver/ToolChains/MSVCSetupApi.h:3
+// Copyright (C) Microsoft Corporation. All rights reserved.
+// Licensed under the MIT license. See LICENSE.txt in the project root for
+// license information.
----------------
dberlin wrote:
> zturner wrote:
> > dberlin wrote:
> > > majnemer wrote:
> > > > dberlin wrote:
> > > > > zturner wrote:
> > > > > > dberlin wrote:
> > > > > > > There is no project root, you should paste the LICENSE.txt into the header directly.
> > > > > > > Also, generally, i would segregate this into a third_party directory, as keeping code like this directly in the rest of the project makes it a lot messier
> > > > > > Is a third_party directory directly in this folder ok, or should it be some kind of top level third_party directory?
> > > > > Normal practice for our open source projects (ie not llvm) would be top-level, because otherwise they tend to be hard to notice.
> > > > > Again, unsure what llvm wants the policy to be here.
> > > > > 
> > > > LLVM, in typical practice, does not segregate small amounts of third party code.
> > > > 
> > > > For example:
> > > > https://github.com/llvm-mirror/llvm/blob/a93bc6c25340329e49407f5fc231ca870f5c61e3/include/llvm/Support/MD5.h
> > > > https://github.com/llvm-mirror/llvm/blob/a93bc6c25340329e49407f5fc231ca870f5c61e3/include/llvm/Support/SHA1.h#L9
> > > > https://github.com/llvm-mirror/llvm/blob/a93bc6c25340329e49407f5fc231ca870f5c61e3/include/llvm/Support/xxhash.h
> > > FWIW, this is likely a remarkably bad idea :)
> > > But i'll email the foundation folks and see what they think.
> > > 
> > Given that there's already precedent for doing de-segregating (is that the right word here?) small pieces of 3rd party code, is it ok for this to go in as-is (after pasting the LICENSE.txt into the header) and then after the foundation reaches a decision, we come back and address it all at once?
> I'm not going to approve it, since i think making the problem worse is also a really bad idea, but if someone else does, and you commit it, i'm not going to take offense.
> 
How about this then?  For now, I will assume that if `_MSC_VER >= 1910` (i.e. you are building with VS 2017), then you also have the Microsoft header, and I will include the Microsoft header and this codepath will work.  Otherwise, you get the old codepath (meaning that if you build clang with MSVC <2017, then you also cannot use the built clang to achieve compatibility with MSVC 2017 even if you copy it to a machine that has MSVC 2017 installed.)

Once you hear back and reach a decision, we can get this header checked in and enable clang-cl bootstrapped with MSVC 2015 to be compatible with MSVC >= 2017.


https://reviews.llvm.org/D30758