Page MenuHomePhabricator

[clangd] Get rid of dots and dotsdots within GlobalCompilationDatabase

Authored by kadircet on Jul 17 2019, 5:57 AM.



These bugs has surfaced during the refactoring D64712.
There are some test cases that passes paths with ".." in them to OverlayCDB and DirectoryBasedCDB, which were not failing before because (tested) users of these functions were handling the canonicalization.

It is also possible to have an LSP client sending over compile commands over the wire without canonicalizing them.
As for the second bug, it is easier to trigger for example a json comp db with an entry ("file": "/abs/path/build/../") will trigger that.

I am not sure if there are build systems generating this kind of output, but it looks valid from a comp db perspective, whereas it doesn't comply with clangd's assumption.

Diff Detail


Event Timeline

kadircet created this revision.Jul 17 2019, 5:57 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 17 2019, 5:57 AM
sammccall added inline comments.Jul 17 2019, 8:42 AM
77 ↗(On Diff #210299)

This patch lacks a bit of context (why are we changing this behavior).
Based on offline discussion, it's not clear to me why this particular change to getFallbackCommand is necessary or desirable. (Compared to others, which seem like fairly obvious bugs, even if the reason to fix them isn't obvious)

144 ↗(On Diff #210299)

nit: can you pull out a local (or FS.h) RemoveDots helper that accepts StringRef and returns std::string?

Most of the usages in this file could be inlined and I think that would read better. Not concerned about performance I think. (But returning SmallString is OK if you are)

307 ↗(On Diff #210299)

Is this actually important/desirable?

kadircet updated this revision to Diff 210354.Jul 17 2019, 9:52 AM
kadircet marked 5 inline comments as done.
  • Add removeDots helper to FS.h
  • Revert changes in getFallbackCommands.
  • Add comments for the reasoning behind removeDots calls.
77 ↗(On Diff #210299)

updating the summary for reasoning

307 ↗(On Diff #210299)

yea agreed this is not necessary.

kadircet edited the summary of this revision. (Show Details)Jul 17 2019, 10:07 AM
sammccall accepted this revision.Jul 18 2019, 7:31 AM

As discussed, the long-term fix might be to avoid ever encountering these paths (filtering everything we get over LSP, CDBs etc).
Maybe add a fixme on removeDots?

This revision is now accepted and ready to land.Jul 18 2019, 7:31 AM
kadircet updated this revision to Diff 210594.Jul 18 2019, 9:10 AM
  • Address comments
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJul 18 2019, 9:14 AM