hamzasood added a comment.
In https://reviews.llvm.org/D28365#665408, @rnk wrote:
> Doesn't your fix mean that the tests will fail on a Windows machine that
> doesn't have VS because LLVM was built with mingw? Usually in these
> situations we provide some way to provide a fake toolchain.
I'm
rnk added a comment.
Doesn't your fix mean that the tests will fail on a Windows machine that
doesn't have VS because LLVM was built with mingw? Usually in these situations
we provide some way to provide a fake toolchain.
https://reviews.llvm.org/D28365
rnk added a comment.
I had to revert this because it doesn't pass tests on Linux. Can you look into
that and resubmit after fixing those test failures?
Repository:
rL LLVM
https://reviews.llvm.org/D28365
___
cfe-commits mailing list
This revision was automatically updated to reflect the committed changes.
Closed by commit rL293923: [Driver] Updated for Visual Studio 2017 (authored by
rnk).
Changed prior to commit:
https://reviews.llvm.org/D28365?vs=86502=86865#toc
Repository:
rL LLVM
https://reviews.llvm.org/D28365
hamzasood added a comment.
In https://reviews.llvm.org/D28365#664892, @rnk wrote:
> This is ready to land. Do you need someone to commit this?
I think so, yeah.
https://reviews.llvm.org/D28365
___
cfe-commits mailing list
rnk added a comment.
This is ready to land. Do you need someone to commit this?
https://reviews.llvm.org/D28365
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
hamzasood updated this revision to Diff 86502.
hamzasood added a comment.
Added a FIXME comment about configuring use of the Setup Configuration API.
In its current form (RC3) the header is distributed in a nuget package, so it's
installed per-project instead of being in a system location that
rnk accepted this revision.
rnk added a comment.
This revision is now accepted and ready to land.
lgtm
Comment at: lib/Driver/MSVCToolChain.cpp:34
+ #if 0
+#define USE_VS_SETUP_CONFIG
+ #endif
hamzasood wrote:
> rnk wrote:
> > What are the outstanding
hamzasood marked 5 inline comments as done.
hamzasood added a comment.
Ping
https://reviews.llvm.org/D28365
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
hamzasood added inline comments.
Comment at: lib/Driver/MSVCToolChain.cpp:34
+ #if 0
+#define USE_VS_SETUP_CONFIG
+ #endif
rnk wrote:
> What are the outstanding issues preventing you from enabling this by default?
Building on Win32 doesn't imply that
hamzasood updated this revision to Diff 84446.
hamzasood added a comment.
Broke up findVCToolChainPath into a few smaller functions.
https://reviews.llvm.org/D28365
Files:
include/clang/Basic/DiagnosticDriverKinds.td
lib/Driver/MSVCToolChain.cpp
lib/Driver/ToolChains.h
rnk added inline comments.
Comment at: lib/Driver/MSVCToolChain.cpp:34
+ #if 0
+#define USE_VS_SETUP_CONFIG
+ #endif
What are the outstanding issues preventing you from enabling this by default?
Comment at:
hamzasood updated this revision to Diff 83943.
hamzasood added a comment.
Uploaded the correct diff this time (yes, really)...
Not sure how to delete the previous one, but it's very incorrect.
https://reviews.llvm.org/D28365
Files:
include/clang/Basic/DiagnosticDriverKinds.td
hamzasood updated this revision to Diff 83941.
hamzasood added a comment.
- Rephrased the diagnostic message (and fixed an embarrassing typo).
- Reverted the linker environment change for now; building for x86 with VS2017
won't work in the meantime. I'll submit it for review separately after
rnk added a comment.
In https://reviews.llvm.org/D28365#640868, @hamzasood wrote:
> Ha, good point. Does that include the environment stuff in Command too or
> just the linker?
Yes, please make the Command environment changes as part of a separate patch
with the linker environment changes.
hamzasood added a comment.
In https://reviews.llvm.org/D28365#640859, @rnk wrote:
> In https://reviews.llvm.org/D28365#640825, @hamzasood wrote:
>
> > In https://reviews.llvm.org/D28365#640775, @rnk wrote:
> >
> > > Can we revert the linker environment change from this patch? It'll be
> > >
rnk added a comment.
In https://reviews.llvm.org/D28365#640825, @hamzasood wrote:
> In https://reviews.llvm.org/D28365#640775, @rnk wrote:
>
> > Can we revert the linker environment change from this patch? It'll be
> > easier to review separately.
>
>
> Sure. But that'll break compiling for x86
hamzasood added a comment.
In https://reviews.llvm.org/D28365#640775, @rnk wrote:
> Can we revert the linker environment change from this patch? It'll be easier
> to review separately.
Sure. But that'll break compiling for x86 on Visual Studio 2017, is that okay?
rnk added a comment.
In https://reviews.llvm.org/D28365#639253, @hamzasood wrote:
> - Added an option to set the environment in a clang::driver::Command, which
> makes the environment modifying introduced in the last update a bit more
> reliable.
>
> @rnk I looked into using the new MSVC
hamzasood added a comment.
In https://reviews.llvm.org/D28365#639260, @awson wrote:
> It's weird th[[ URL | name ]]at cl.exe command-line compiler reports version
> 19.10.24629 and lives in the 14.10.24629 directory (only first 2 digits are
> different).
>
> Moreover, in their explanation
awson added a comment.
It's weird that cl.exe command-line compiler reports version 19.10.24629 and
lives in the 14.10.24629 directory (only first 2 digits are different).
Moreover, in their explanation blogpost
hamzasood updated this revision to Diff 83555.
hamzasood added a comment.
- Added an option to set the environment in a clang::driver::Command, which
makes the environment modifying introduced in the last update a bit more
reliable.
@rnk I looked into using the new MSVC toolchain layout to get
hamzasood updated this revision to Diff 83473.
hamzasood added a comment.
- Modify the environment in some cases in buildLinker to keep link.exe happy.
Thanks @rnk for taking the time to look at this. As suggested:
- Replaced the confusing unique_ptr usage with a struct.
- Renamed
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
hamzasood updated this revision to Diff 83325.
hamzasood added a comment.
Improved the code slightly.
Sorry for the spam everyone, this is definitely the one.
https://reviews.llvm.org/D28365
Files:
include/clang/Basic/DiagnosticDriverKinds.td
lib/Driver/MSVCToolChain.cpp
hamzasood updated this revision to Diff 83318.
hamzasood added a comment.
- Re-implemented the PATH searching behaviour (thanks @amccarth for pointing
that out)
- Updated the base revision.
https://reviews.llvm.org/D28365
Files:
include/clang/Basic/DiagnosticDriverKinds.td
amccarth added a comment.
(I could be missing something because the diff doesn't have much context.)
If I'm reading this right, it looks like it will no longer search the PATH in
order to find cl.exe. If that's the case, then I'm mildly worried about this
change in behavior. I know I have
hamzasood updated this revision to Diff 83303.
hamzasood added a comment.
- Fixed an error in retrieving a toolchain path from the registry.
- Updated the base revision.
https://reviews.llvm.org/D28365
Files:
include/clang/Basic/DiagnosticDriverKinds.td
lib/Driver/MSVCToolChain.cpp
hamzasood created this revision.
hamzasood added reviewers: rnk, ruiu, hans.
hamzasood added a subscriber: cfe-commits.
The patch updates the MSVC ToolChain for the changes made in Visual Studio 2017
(https://blogs.msdn.microsoft.com/vcblog/2016/10/07/compiler-tools-layout-in-visual-studio-15/).
29 matches
Mail list logo
