[PATCH] D28365: [Driver] Updated for Visual Studio 2017

Reid Kleckner via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Mon Jan 30 16:16:41 PST 2017


rnk accepted this revision.
rnk added a comment.
This revision is now accepted and ready to land.

lgtm



================
Comment at: lib/Driver/MSVCToolChain.cpp:34
+  #if 0
+    #define USE_VS_SETUP_CONFIG
+  #endif
----------------
hamzasood wrote:
> rnk wrote:
> > What are the outstanding issues preventing you from enabling this by default?
> Building on Win32 doesn't imply that you'll have the required header;  it currently needs to be installed [[ https://blogs.msdn.microsoft.com/heaths/2016/09/15/changes-to-visual-studio-15-setup/ | separately ]] via nuget. While it would be possible to have cmake create a packages.config file when configuring a visual studio project, the API is only at the RC stage and so the distribution method could potentially change between now and the Visual Studio 2017 release (even if that's not a concern, it's probably out of the scope of this patch anyway).
> Although the code looks useless sitting there ifdefed out, it could be useful for someone eager enough to get the package themselves and run a custom Clang build.
> In the meantime, Visual Studio 2017 installations can only be detected when running in the correct developer command prompt, or by putting one of its toolchain's bin directories at the top of PATH.
This patch looks good, so I don't want to block it, but can you add something like:
 // FIXME: Use cmake to auto-detect if the necessary headers exist.
I'm assuming we should test for <Setup.Configuration.h> eventually.


https://reviews.llvm.org/D28365





More information about the cfe-commits mailing list