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
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
