Page MenuHomePhabricator

[clangd] Unify path canonicalizations in the codebase

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



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.

Diff Detail


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.

91 ↗(On Diff #178635)

A different name proposal: canonicalizePath? WDYT?

91 ↗(On Diff #178635)

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

58 ↗(On Diff #178635)

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.

62 ↗(On Diff #178635)

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

64 ↗(On Diff #178635)

Why not do this only in the else branch?

kadircet marked 7 inline comments as done.Dec 18 2018, 8:15 AM
kadircet added inline comments.
91 ↗(On Diff #178635)

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

58 ↗(On Diff #178635)

As discussed offline this is not introducing a behavior change.

64 ↗(On Diff #178635)

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.
203 ↗(On Diff #178685)

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

205 ↗(On Diff #178685)

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

91 ↗(On Diff #178635)

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

58 ↗(On Diff #178635)

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.