mstorsjo added a subscriber: rnk.
mstorsjo added a comment.
FWIW, this change is not about mingw, it's about making the
windows-with-forward-slashes configuration usable.
The windows-with-forward-slashes configuration works just as fine in MSVC
configurations, and that's where I've tested it. A MSVC build with `ninja
check-all` that succeeded before, still succeed with `-DLLVM_
WINDOWS_PREFER_FORWARD_SLASH=ON` (with respect to clangd) after this change.
As for testing strategy, it could be concievable to add a configuration to e.g.
the buildkite premerge tests that build+test everything in a
`-DLLVM_WINDOWS_PREFER_FORWARD_SLASH=ON` environment too. (I've seen interest
from @rnk to push towards such a setup, possibly, so adding testing
configurations for both modes is probably not inconcievable.)
================
Comment at: clang-tools-extra/clangd/tool/ClangdMain.cpp:546
fs::make_absolute(TestScheme::TestDir, Path);
+ path::native(Path);
return std::string(Path);
----------------
sammccall wrote:
> This change is an example of something subtle that I don't understand, and
> don't expect other maintainers to understand, which is therefore fragile.
Hmm, I'll have to revisit this particular change and see if it really was
necessary in the end, and add a comment explaining the subtlety.
================
Comment at: clang-tools-extra/clangd/unittests/TestFS.h:79
+llvm::SmallString<16> testRootBuf();
+#define testRoot() testRootBuf().c_str()
----------------
sammccall wrote:
> If we want to make this more broadly compatible, we'd need to finder
> cleaner/less invasive ways...
Sure, this is admittedly a bit hacky, but managed to make tests pass with a
fairly minimal amount of changes.
================
Comment at: clang-tools-extra/clangd/unittests/URITests.cpp:137
+ llvm::SmallString<16> Ref("c:\\x\\y\\z");
+ llvm::sys::path::native(Ref);
+ EXPECT_THAT(resolveOrDie(parseOrDie("file:///c%3a/x/y/z")), Ref);
----------------
sammccall wrote:
> This seems incorrect. Any build running on windows should be able to handle
> backslash paths read from compile_commands.json etc.
>
>
Yes, but that's not what the changes does. Regardless of the preference for
forward or backwards slashes, both configurations (at least when it comes to
LLVMSupport general functions) support both types of paths, that's not changed.
Previously, this test verified that if you resolve `file:///c%a/x/y/z` you get
`c:\\x\\y\\z`. Now in a forward slash environment, you get `c:/x/y/z` - which
is expected.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D113268/new/
https://reviews.llvm.org/D113268
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
