This is an archive of the discontinued LLVM Phabricator instance.

[clangd] Unify path canonicalizations in the codebase
ClosedPublic

Authored by kadircet on Dec 18 2018, 3:34 AM.

Details

Summary

There were a few different places where we canonicalized paths, each
one had its own flavor. This patch tries to unify them all under one place.

Event Timeline

kadircet created this revision.Dec 18 2018, 3:34 AM
ilya-biryukov added a comment.EditedDec 18 2018, 6:27 AM

The change mostly LG, the only important comment from me is about toURI. I believe it would actually have an observable effect in the real world and I'm not sure things won't break after it.
Could we avoid changing it and focus only on tweaking the "canonical" path for the FileEntries in this change? Those look like a no-op in most practical cases.

clangd/SourceCode.h
91

A different name proposal: canonicalizePath? WDYT?

91

Maybe leave it Optional?
Semantic differences make it very hard to verify the code after rewrite is correct, as Expected requires checking for errors.

clangd/index/SymbolCollector.cpp
58

This looks like the only change that might subtly change the behavior of our program. I believe things might break because of this.
Could we keep this review closer to an NFC and avoid introducing this subtle difference here? Happy to take a look at a separate patch with it.

61–64

Why not do this only in the else branch?

62

This comment looks useful. Maybe keep it in the new implementation?

kadircet marked 7 inline comments as done.Dec 18 2018, 8:15 AM
kadircet added inline comments.
clangd/SourceCode.h
91

but it rather gets the canonical path for a FileEntry rather than canonicalizing a path ?

clangd/index/SymbolCollector.cpp
58

As discussed offline this is not introducing a behavior change.

61–64

Adding a comment.

kadircet updated this revision to Diff 178685.Dec 18 2018, 8:15 AM
kadircet marked 2 inline comments as done.
  • Address comments
ilya-biryukov accepted this revision.Dec 19 2018, 2:06 AM
ilya-biryukov added inline comments.
clangd/SourceCode.cpp
209

Using : twice is a bit hard to read.
Maybe use something like failed to turn relative path '{0}' to absolute: {1} instead?

211

NIT: remove llvm:: prefix in a cpp file. Same for other occurences.

clangd/SourceCode.h
91

SG, not a big deal anyway. I have a personal bias against methods starting with get if they're not getter xD)

clangd/index/SymbolCollector.cpp
58

Thanks for pointing me to the code I've missed

This revision is now accepted and ready to land.Dec 19 2018, 2:06 AM
kadircet updated this revision to Diff 178856.Dec 19 2018, 2:48 AM
kadircet marked 3 inline comments as done.
  • Address comments
This revision was automatically updated to reflect the committed changes.