[PATCH] D113268: [clangd] Fix tests with forward slash as separator on windows

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

In D113268#3111709 <https://reviews.llvm.org/D113268#3111709>, @mstorsjo wrote:

> FWIW, this change is not about mingw, it's about making the windows-with-forward-slashes configuration usable.

OK - can I ask why the windows-with-forward-slashes configuration is valuable to support?
I was assuming it was mostly useful to folks building with mingw, sounds like that's not all!

> 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.)

That's great to hear. It'd really be a prerequisite to keeping this configuration passing, as we don't have regular access to windows machines & rely on the buildbots to catch test problems.

I think the biggest testing problem we have though is that we don't have a good way of defining integration tests with paths set up the right way (since we can't really use absolute paths in our tests, and most of our bugs involve comparing absolute paths).



================
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);
----------------
mstorsjo wrote:
> 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.
Of course, sorry!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D113268/new/

https://reviews.llvm.org/D113268