[PATCH] D20136: Get default -fms-compatibility-version from cl.exe's version

[Reid Kleckner via cfe-commits]

rnk added inline comments.

================
Comment at: lib/Driver/MSVCToolChain.cpp:41
@@ -40,1 +40,3 @@
+
+  #pragma comment(lib, "version.lib")
 #endif
----------------
Personally, I think this is OK but I know Aaron Ballman and other people don't like using pragma comment lib. The alternative is hacking CMake goo, which is always best avoided when possible.

================
Comment at: lib/Driver/MSVCToolChain.cpp:472
@@ +471,3 @@
+
+  const DWORD VersionSize = ::GetFileVersionInfoSizeA(ClExe.c_str(), nullptr);
+  if (VersionSize == 0) {
----------------
majnemer wrote:
> amccarth wrote:
> > majnemer wrote:
> > > Why not use the `GetFileVersionInfoSizeW` variant?
> > I started down that road, but it seemed overkill to convert the path to a wide string.  I'm happy to do it if you think it worthwhile.
> I think it's worth it, we get bug reports whenever we break this sort of thing...
+1, you can use ConvertUTF8toWide to make this easy.

================
Comment at: lib/Driver/MSVCToolChain.cpp:477
@@ +476,3 @@
+  std::vector<char> VersionBlock(VersionSize);
+  if (!::GetFileVersionInfoA(ClExe.c_str(), 0, VersionSize,
+                             VersionBlock.data())) {
----------------
majnemer wrote:
> amccarth wrote:
> > thakis wrote:
> > > We already stat a bunch of directories to find the sdk include path. Can we use the result of that instead of looking at cl.exe? Then we wouldn't have to do additional stats.
> > I'm just a simple CPM/VMS/Windows developer.  Your Linux terms (stat) frighten and confuse me.
> > 
> > Seriously, though, this API isn't a file system check.  It's digging out the version record from the file's resources.
> > 
> > We _could_ guess at the version from the names of the directories in the path, but that would require mapping names to versions, and if it's installed in a non-standard place it wouldn't help at all.
> > 
> > Also, `-fms-compatibility-version` is really about the version of the compiler (cl.exe), not that of the standard library nor of the SDK, so trying to check something else as a proxy for the version seems prone to obscure failures.
> > 
> > I share your concern about speed, especially since getting the version happens twice (once for the triple and once for the compatibility version), but invoking clang and having it choose the wrong default costs a lot of time, too.
> > 
> > The bug report correctly says we shouldn't spin up a process to run `cl /version`--that would be far more expensive.  And if you put `-fms-compatibility-version` on the command line, then this function won't be called as it won't need to figure out the default.
> > Seriously, though, this API isn't a file system check. It's digging out the version record from the file's resources.
> 
> Isn't the content stored as a resource in the PE?  If so, that means that getting the version information requires reading bytes inside of cl.exe
> 
> With regard to `-fms-compatibility-version`, it shouldn't have anything to do with the platform SDK.  However, it is fundamentally the case that the CRT and the compiler have the same version.  Otherwise, really terrible things happen (MSVC19 assumes char16_t is a keyword which it provides but the MSVC18 STL assumes it is responsible for providing the keyword).
I think one extra file read is probably worth the convenience it buys for our users. It's easy to win back by having the user pass an explicit -fms-compatibility-version flag.

================
Comment at: lib/Driver/Tools.cpp:3337-3338
@@ +3336,4 @@
+    if (IsWindowsMSVC) {
+      const auto &MSVC = static_cast<const toolchains::MSVCToolChain &>(TC);
+      VersionTuple MSVT = MSVC.getMSVCVersionFromExe();
+      if (!MSVT.empty())
----------------
IMO you should make this a virtual method on Toolchain that does nothing and is only overridden in MSVCToolChain. You can also cache it if you do that.

================
Comment at: tools/driver/driver.cpp:504
@@ -503,3 +503,3 @@
   // Exit status should not be negative on Win32, unless abnormal termination.
-  // Once abnormal termiation was caught, negative status should not be
+  // Once abnormal termination was caught, negative status should not be
   // propagated.
----------------
Yeah, it's a typo, but you don't have any other changes in this file, so I wouldn't touch it as part of this change.


http://reviews.llvm.org/D20136