sammccall accepted this revision. sammccall added a comment. This revision is now accepted and ready to land.
LG. A couple of optional comments to address as you see fit. It looks like you're not an LLVM committer yet, do you want me to commit this for you? (Usually it's reasonable to ask for this after landing a couple of patches: https://llvm.org/docs/DeveloperPolicy.html#obtaining-commit-access) ================ Comment at: clang-tools-extra/clangd/PathMapping.cpp:153 +// Converts \pPath to a posix-style absolute, i.e. if it's a windows path +// then the backward-slash notation will be converted to forward slash +llvm::Expected<std::string> parsePath(llvm::StringRef Path) { ---------------- wwagner19 wrote: > sammccall wrote: > > "posix-style" doesn't really describe representing `c:\foo` as `/c:/foo`. > > That's really *just* a URI thing AFAIK. > > > > Something like "Converts a unix/windows path to the path part of a file > > URI". > > But in that case, I think the implementation is just > > `URI::createFile(Path).body()`. Does that pass tests? > Oh I did not realize `createFile` was a thing, however looking at the way > it's implemented now, won't that throw an assert if a non-absolute path is > passed in? If so, is that desirable at all? > > IIUC, if I were to use that API, then wouldn't it make more sense for it to > return an `llvm::Expected`? If we want to consolidate the logic to one place, > I'd be happy to try and refactor the signature. I think allowing relative paths to be specified in path mappings is needlessly confusing. Clangd flags are typically specified in a config file somewhere, and that config often doesn't know about the working directory. We don't want to hit the assert, so I'd suggest testing if it's absolute first, and returning an error if not. ================ Comment at: clang-tools-extra/clangd/PathMapping.cpp:214 + llvm::SmallString<256> MappedPath(Uri->body()); + llvm::sys::path::replace_path_prefix(MappedPath, From, To); + auto MappedUri = URI(Uri->scheme(), Uri->authority(), MappedPath.c_str()); ---------------- wwagner19 wrote: > sammccall wrote: > > Sorry, I know I suggested replace_path_prefix, but now that the mappings > > consist of paths in their URL form, I think the old string::replace version > > is what you want :-/ > Will do, I like string replace better anyway! > > I'm not a huge fan of the `mappingMatches` function, and would prefer a > simple string `startswith(from)`, but the only way I may see that working is > by appending "/" to all the path mappings internally - which would prevent > `/foo` matching `/foo-bar` - but appending a "/" would break directory-based > file URIs, I believe. Yeah, I see what you mean. My favorite way to structure this code would probably be to always **strip** trailing slashes from the mappings, and combine the check-for-match and apply-match for a single entry: ```optional<std::string> doPathMapping(StringRef Path, StringRef From, StringRef To) { if (Path.consume_front(From) && (Path.empty() || Path.startswith('/')) return (To + Path).str() return None; }``` Totally up to you, your tests look good :-) CHANGES SINCE LAST ACTION https://reviews.llvm.org/D64305/new/ https://reviews.llvm.org/D64305 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
