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
