[PATCH] D44158: [clangd:vscode] Resolve symlinks for file paths from clangd.

[Eric Liu via Phabricator via cfe-commits]

ioeric added inline comments.


================
Comment at: clangd/clients/clangd-vscode/src/extension.ts:3
 import * as vscodelc from 'vscode-languageclient';
+import { realpathSync } from 'fs';
 
----------------
sammccall wrote:
> nit: the braces don't do anything here, right?
I'm not sure... this was added by vscode. 


================
Comment at: clangd/clients/clangd-vscode/src/extension.ts:28
     if (!!traceFile) {
-      const trace = {CLANGD_TRACE : traceFile};
-      clangd.options = {env : {...process.env, ...trace}};
+        const trace = { CLANGD_TRACE: traceFile };
+        clangd.options = { env: { ...process.env, ...trace } };
----------------
sammccall wrote:
> (this is an unrelated formatting change, so consider reverting anyway)
> My clang-format doesn't add the whitespace inside {}, and the indentation change seems just wrong... what are you using to format?
This was also vscode which formats a whole file by default. Yeah, it's unrelated, but it's a bit annoying to revert all formatting changes...

Regarding the indentation, I think it just follows the current indentation in the file.


================
Comment at: clangd/clients/clangd-vscode/src/extension.ts:41
+        },
+        // Resolve symlinks for all files provided by clangd.
+        // FIXME: it might make more sense to rely on clangd to handle symlinks.
----------------
sammccall wrote:
> I think this comment needs to be more precise, because it's important we know when to remove this.
> 
> This is a workaround for a bazel + clangd issue - bazel produces a symlink tree to build in, and  when navigating to the included file, clangd passes its path inside the symlink tree rather than its filesystem path. We work around this by resolving all symlinks on the client.
> We should remove this once clangd knows enough about bazel to resolve the symlinks where needed (or if this causes problems for other workflows).
> 
> (that's not necessarily text that needs to go into the comment, but I think all the information does...)
Done. Thanks!


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D44158