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

[Reid Kleckner via Phabricator via cfe-commits]

rnk added a comment.

The new MSVC layout suggests to me that we should look try looking at the path of cl.exe before we open the exe and try to check its version. We'd change this code to inspect the path before looking in the exe:

  VersionTuple MSVCToolChain::computeMSVCVersion(const Driver *D,
                                                 const ArgList &Args) const {
    bool IsWindowsMSVC = getTriple().isWindowsMSVCEnvironment();
    VersionTuple MSVT = ToolChain::computeMSVCVersion(D, Args);
    if (MSVT.empty()) MSVT = getMSVCVersionFromTriple();
    if (MSVT.empty() && IsWindowsMSVC) MSVT = getMSVCVersionFromExe();
    if (MSVT.empty() &&
        Args.hasFlag(options::OPT_fms_extensions, options::OPT_fno_ms_extensions,
                     IsWindowsMSVC)) {
      // -fms-compatibility-version=18.00 is default.
      // FIXME: Consider bumping this to 19 (MSVC2015) soon.
      MSVT = VersionTuple(18);
    }
    return MSVT;
  }

That can definitely be a separate change, though.



================
Comment at: lib/Driver/MSVCToolChain.cpp:76
+// Attempts to find the "best" usable VC toolchain.
+static bool findVCToolChainPath(std::string &Path, bool &IsVS2017OrNewer) {
+  // Check the environment first, since that's probably the user telling us
----------------
Thanks for rewriting this, the logic here seems much clearer.


================
Comment at: lib/Driver/MSVCToolChain.cpp:186
+    // this scope.
+    _set_com_error_handler([](HRESULT, IErrorInfo *) { });
+    std::unique_ptr<decltype(*_com_raise_error),
----------------
Is this really supported? Are these libraries prepared to return failure HRESULT values if an exception isn't thrown?

Also, won't this suppress exceptions that would normally be internal to COM, or does that never happen?


================
Comment at: lib/Driver/MSVCToolChain.cpp:187
+    _set_com_error_handler([](HRESULT, IErrorInfo *) { });
+    std::unique_ptr<decltype(*_com_raise_error),
+                    decltype(&_set_com_error_handler)>
----------------
This is a very surprising use of unique_ptr. I think it would be clearer written this way:
  struct RestoreCOMErrorHandler {
    ~RestoreCOMErrorHandler() { _set_com_error_handler(_com_raise_error); }
  } COMErrorRestorer;


================
Comment at: lib/Driver/MSVCToolChain.cpp:352
+// to the corresponding subdirectory name.
+static const char *llvmArchToSubDirectoryName(llvm::Triple::ArchType Arch) {
+  using ArchType = llvm::Triple::ArchType;
----------------
Think there's a better name for this than "SubDirectoryName"? It sounds like these are common to the Win8+ SDk and the VC2017+ tools. We could still call it the "WindowsSDKArch", since that's the package that initially adopted this naming convention.


================
Comment at: lib/Driver/MSVCToolChain.cpp:370
+std::string MSVCToolChain::getSubDirectoryPath(SubDirectoryType Type) const {
+  auto llvmArchToLegacyVCSubDirectoryName =
+      [](llvm::Triple::ArchType Arch) -> const char * {
----------------
For consistency, please make this a static helper like the helper above.


https://reviews.llvm.org/D28365