[PATCH] D71029: [clangd] (take 2) Try harder to find a plausible `clang` as argv0, particularly on Mac.

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

Thanks for looking at this Jan, I should have put you on the reviewers in the first place...



================
Comment at: clang-tools-extra/clangd/CompileCommands.cpp:53
+  }
+  StringRef Path = Buf->get()->getBuffer().trim();
+  if (Path.empty()) {
----------------
jkorous wrote:
> The `trim()` is probably safe - just wondering if that could cause issues in some weird case.
Only if the path ends with a space, AFAICS. I don't think it's worth worrying about.


================
Comment at: clang-tools-extra/clangd/CompileCommands.cpp:65
+  if (llvm::sys::fs::real_path(Path, Resolved))
+    return Path; // On error;
+  return Resolved.str();
----------------
jkorous wrote:
> Just wondering why you decided to not somehow propagate the error - are there scenarios where the path is correct yet not resolvable on real FS?
By definition probably not *correct* cases, but this will be hit if /usr/bin/clang is a dangling symlink, or xcrun lies to us, etc.

There's no error *propagation* because error recovery is here: even if we can't work out whether clang is a symlink, the best thing to do is create a mangler that assumes it isn't. (Or maybe better to create a mangler that ignores this path entirely, but I'm not convinced).
There's no value in knowing about this failure at the site where the mangler is created, or where the compile command is mangled, or where the file is opened - we're always going to want to recover and forge ahead, and we have the best information to recover here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71029