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

[Martin Storsjö via Phabricator via cfe-commits]

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