[PATCH] D130690: [clangd][NFCI] Store TUPath inside ParsedAST

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet marked 2 inline comments as done.
kadircet added a comment.

as discussed offline, this patch stores the filepath provided by the client (e.g. vscode) which is usually the filepath seen by the user, inside the ParsedAST and later on uses that when a hint is needed for translating filepaths coming from sourcemanager into uris.
this gets rid of the extra canonicalization needed for the main file path (which could fail when we don't have a current working directory set, i.e. in tests) and also does always the "right" thing as all the files should be relative to the workspace of the main file path seen by the user, not the version clangd derives somehow.



================
Comment at: clang-tools-extra/clangd/ParsedAST.cpp:763
 
-ParsedAST::ParsedAST(llvm::StringRef Version,
+ParsedAST::ParsedAST(PathRef TUPath, llvm::StringRef Version,
                      std::shared_ptr<const PreambleData> Preamble,
----------------
ilya-biryukov wrote:
> NIT: use `Path`.
>  this allows the callers to avoid an extra allocation, e.g. if they already allocated a `std::string` and they can `std::move` into the constructor.
this is a private constructor and public interface also receives a stringref and none of the production callers actually have an extra copy of the filename lying around. so i'd rather keep it as is.


================
Comment at: clang-tools-extra/clangd/ParsedAST.h:112
 
+  /// Returns the path passed by the caller when building this AST.
+  PathRef tuPath() const { return TUPath; }
----------------
ilya-biryukov wrote:
> NIT: could you add a comment how this is used, e.g. `this path is used as a hint when canonicalize the path`.
> Also useful to document that we expect this to be absolute/"canonical".
i'd rather not complicate the wording here.  i think mentioning the fact that this is the filepath as provided by the caller is enough. being used as a hint for canonicalization of other paths is something specific to the application (and is a result of being the path provided by caller anyway.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130690