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

Mon Nov 22 03:32:07 PST 2021

mstorsjo added a comment.

Sorry for taking so long to get back to you on this matter...

In D113268#3112026 <https://reviews.llvm.org/D113268#3112026>, @sammccall wrote:

>> For someone unfamiliar with the tool, is there a simple "smoke test procedure" I could try out with it to kick the tyres?
>
> There's `clangd --check=some_file.cc` which simulates opening up a file in an editor and clicking around in it.
> It does require a `compile_command.json` in a parent directory to know about build flags.
> To try it out for real you need to connect an editor to it with a plugin (like vscode-clangd).

Thanks! I tested both `clangd --check=some_file.cc` and vscode-clangd with a build of clangd set to prefer forward slashes, and it all worked fine so far. I didn't do any exhaustive use of it in vscode, but at least regular use seemed just fine.

> But realistic problems with paths tend to come from interactions with external sources.
> For example, we had a bug that involved:
>
> - vscode sending paths like `file://c:/test.cc` (lowercase C), which made its way into our AST cache
> - cmake creating `compile_commands.json` like `C:\test.cc` (uppercase C), which made its way into our index
> - when renaming a symbol, results from the two weren't deduplicated against each other so the edit was applied twice

Yup - I've seen lots of similar issues in other clang tests too. The best way forward to fix those would probably be to add some helper to `llvm::sys::path` for doing path comparisons, which would ignore case differences (on windows and macos) and/or separator style differences (on windows). But

Anyway, this change (and a build that prefers generating paths with forward slashes) seems to work as expected with regards to that - and I updated the patch with what was requested.

The whole setup for this mode isn't settled, so there's no buildbot for this mode (yet), we'll see if we end up setting one up. In any case, most of this change shouldn't be too intrusive I hope.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113268