majnemer added inline comments. ================ Comment at: lib/Driver/MSVCToolChain.cpp:472 @@ +471,3 @@ + + const DWORD VersionSize = ::GetFileVersionInfoSizeA(ClExe.c_str(), nullptr); + if (VersionSize == 0) { ---------------- 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...
================ Comment at: lib/Driver/MSVCToolChain.cpp:477 @@ +476,3 @@ + std::vector<char> VersionBlock(VersionSize); + if (!::GetFileVersionInfoA(ClExe.c_str(), 0, VersionSize, + VersionBlock.data())) { ---------------- 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). http://reviews.llvm.org/D20136 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits