[PATCH] D64305: [clangd] Add path mappings functionality

This revision was automatically updated to reflect the committed changes. Closed by commit rGc69ae835d0e0: [clangd] Add path mappings functionality (authored by sammccall). Changed prior to commit: https://reviews.llvm.org/D64305?vs=236295=236548#toc Repository: rG LLVM Github Monorepo

[PATCH] D64305: [clangd] Add path mappings functionality

sammccall added a comment. Thanks! The latest snapshot looks good. Landing it now with a few minor tweaks mentioned below. (And trivial local style things, we generally prefer to drop braces on simple if statements etc.) Comment at:

[PATCH] D64305: [clangd] Add path mappings functionality

wwagner19 updated this revision to Diff 236295. wwagner19 added a comment. To be honest, I'm not sure how to remedy this. So I just rebased all my commits into one and dropped the `git show HEAD -U99` into here. Please let me know if you need me to fix anything / open a new diff. CHANGES

[PATCH] D64305: [clangd] Add path mappings functionality

wwagner19 updated this revision to Diff 236293. wwagner19 added a comment. The last diff was broken, this most recent one - Changes IsIncoming boolean to an enum - Refactors the matching path logic CHANGES SINCE LAST ACTION https://reviews.llvm.org/D64305/new/

[PATCH] D64305: [clangd] Add path mappings functionality

sammccall added a comment. Sorry, I was out for a bit and this fell off my radar. It looks good, but the last snapshot seems to have reverted some earlier changes (book vs enum, some doxygen comments etc). Do you know what's up there? Either way I should be able to apply this on Monday

[PATCH] D64305: [clangd] Add path mappings functionality

wwagner19 added a comment. Hey @sammccall, any update here? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D64305/new/ https://reviews.llvm.org/D64305 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D64305: [clangd] Add path mappings functionality

wwagner19 marked 2 inline comments as done. wwagner19 added inline comments. 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

[PATCH] D64305: [clangd] Add path mappings functionality

wwagner19 updated this revision to Diff 229969. wwagner19 marked 2 inline comments as done. wwagner19 added a comment. Awesome! I am not an LLVM committer, so if you could commit on my behalf that'd be great- although I'm not sure how LLVM handles squashing/merging, continuous integration,

[PATCH] D64305: [clangd] Add path mappings functionality

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

[PATCH] D64305: [clangd] Add path mappings functionality

wwagner19 marked 9 inline comments as done and an inline comment as not done. wwagner19 added inline comments. 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

[PATCH] D64305: [clangd] Add path mappings functionality

wwagner19 updated this revision to Diff 229721. wwagner19 marked 5 inline comments as done. wwagner19 added a comment. Thanks for the second review Sam, I addressed most of your comments, notably: - Changed the bool IsIncoming to an enum - Fixed the "doxygen" comments, - Removed some redundant

[PATCH] D64305: [clangd] Add path mappings functionality

sammccall added a comment. Thanks, this looks a lot better. Main thing that was unclear to me is the fact that the paths in the mappings are now URI-paths, so "/C:/foo" on windows. This looks like the right idea as it ensures much of the path-mapping code gets to ignore slashes and such.

[PATCH] D64305: [clangd] Add path mappings functionality

sammccall added a comment. (also welcome back and thanks for picking this up!) CHANGES SINCE LAST ACTION https://reviews.llvm.org/D64305/new/ https://reviews.llvm.org/D64305 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D64305: [clangd] Add path mappings functionality

wwagner19 updated this revision to Diff 227643. wwagner19 added a comment. Herald added a subscriber: usaxena95. Unfortunately, I had to take a bit of a hiatus there, but i'm back a few months later with an updated patch incorporating all of @sammccall 's feedback! Notably, - Windows paths are

[PATCH] D64305: [clangd] Add path mappings functionality

wwagner19 marked 9 inline comments as done. wwagner19 added a comment. Thanks for all the feedback, Sam! I'll try and get an updated patch up sometime tomorrow. Comment at: clang-tools-extra/clangd/PathMapping.cpp:30 + const auto = V.kind(); + if (K == Kind::Object) { +

[PATCH] D64305: [clangd] Add path mappings functionality

sammccall added a comment. Thanks for putting this together. Overall it looks pretty good! Main issues to address are: - file path handling should manage windows paths and have tests for this - the lit test can be simplified quite a lot I think Comment at:

[PATCH] D64305: [clangd] Add path mappings functionality

wwagner19 marked 2 inline comments as done. wwagner19 added a comment. Herald added a subscriber: ormris. Hey, This is my first proposed change to LLVM, so sorry if I messed anything up. The proposed changes here follow from discussion on clangd-dev (from janruary

[PATCH] D64305: [clangd] Add path mappings functionality

wwagner19 created this revision. wwagner19 added reviewers: sammccall, ilya-biryukov. Herald added subscribers: cfe-commits, kadircet, arphaman, jkorous, MaskRay, mgorny. Herald added a project: clang. Add path mappings to clangd which translate file URIs on inbound and outbound LSP messages.