mstorsjo added inline comments.
================
Comment at: clang/lib/Driver/ToolChains/MinGW.h:63
+ static void FixTripleArch(const Driver &D, llvm::Triple &Triple,
+ const llvm::opt::ArgList &Args);
----------------
MaskRay wrote:
> Use `functionName` for newer functions. (It's unfortunate that Clang has much
> inconsistency but we should do the right thing for new methods.)
Sure, will change it.
================
Comment at: clang/test/Driver/mingw-sysroot.cpp:49
+
+// RUN: env "PATH=%T/testroot-gcc/bin:%PATH%"
%T/testroot-clang/bin/x86_64-w64-mingw32-clang -target x86_64-w64-mingw32 -m32
-rtlib=compiler-rt -stdlib=libstdc++ --sysroot="" -c -### %s 2>&1 | FileCheck
-check-prefix=CHECK_TESTROOT_CLANG_I686 %s
+// CHECK_TESTROOT_CLANG_I686:
"{{[^"]+}}/testroot-clang{{/|\\\\}}i686-w64-mingw32{{/|\\\\}}include"
----------------
MaskRay wrote:
> Changing `PATH` seems unreliable for some test environments.
>
> Any alternative way to test this?
>
> `--target=` is preferred over `-target ` for driver options. `-target ` is
> legacy but we probably need to keep indefinitely for its wide usage.
>
> `linux-cross.cpp` has some nice tests which may inspire this file ? :)
Using `PATH` isn't very nice, no, but it's at least reliable enough to run on
the current testing setups?
I don't see any good input for testing this in linux-cross.cpp - that file uses
explicit `--sysroot=` parameters all over the place, while this test file is
mostly about how to best implicitly deduce the sysroot when one isn't specified.
This file (both the old and the newly added tests) relies on checking what
directories it finds in `<clang-executable>/../<triple>`, so it requires
fake-installing the clang binary by symlinking into a test toolchain tree.
I guess it could be possible to execute that fake-installed clang binary by
specifying the direct path to it, instead of adding it to `PATH` - but it's an
important usecase to make sure that it works when the literal path isn't
spelled out in `argv[0]` too.
I can change to use `--target=` in the added lines, but the rest of the file
uses the other spelling anyway (and I don't think it's on topic to do such
rewrites as part of this patch).
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D111952/new/
https://reviews.llvm.org/D111952
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
