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

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

TL;DR: clangd is unsupported/broken on mingw, this patch only makes the tests pass.
The current maintainers don't plan to invest in this, or accept much extra complexity to support it.
If you or someone wants to find a clean way to support the configuration, it'd be great. Otherwise making the tests pass has little intrinsic value.

---

It sounds like this is the build option `WINDOWS_PREFER_FORWARD_SLASH_DEFAULT`, which is set for mingw builds (and could be set manually too).

I think the current state is:

- clangd+mingw is effectively unsupported (no bots, no official binaries, no devs who can verify bugs)
- clangd has path-handling problems when built under mingw.
- this patch doesn't fix clangd, it only makes the tests pass. Our tests mostly attempt to be "cross-platform", path-handling problems turn up as weird interactions between tools that get polished though bug reports.

Supporting windows-forward-slash paths would probably be a quite an effort (based on the effort of supporting "standard" windows paths).
This includes code changes, setting up testing infrastructure, refining behavior through bug reports, the ongoing cost of dealing with more complex/subtle path handling code.

I'm not sure this effort is justified by the impact. I'm open to ideas about how to support this but can't dedicate my own time to it. I am opposed to making the code much more complex/fragile to deal with it.
If someone's interested in maintaining and testing this configuration and can find good ways to encapsulate the extra complexity, it'd be great to support this.
Unless we have a plan to support it I don't think we should go to any lengths to make the tests pass.

(The current state of "it builds but doesn't work or pass tests" is bad, maybe we should be disabling clangd in CMake like we do when threads are disabled).



================
Comment at: clang-tools-extra/clangd/tool/ClangdMain.cpp:546
     fs::make_absolute(TestScheme::TestDir, Path);
+    path::native(Path);
     return std::string(Path);
----------------
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.


================
Comment at: clang-tools-extra/clangd/unittests/TestFS.h:79
+llvm::SmallString<16> testRootBuf();
+#define testRoot() testRootBuf().c_str()
 
----------------
If we want to make this more broadly compatible, we'd need to finder cleaner/less invasive ways...


================
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);
----------------
This seems incorrect. Any build running on windows should be able to handle backslash paths read from compile_commands.json etc.




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113268