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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
