[WIP] make KDevelop::Path work with canonical representations
AbandonedPublic

Authored by rjvbb on Sep 23 2017, 1:06 PM.

Details

Reviewers
mwolff
Group Reviewers
KDevelop
Summary

This is a spin-off of D7930, intended to investigate the idea of working with canonical path representations throughout KDevelop.

The patch achieves this by converting paths to their canonical representation on input, currently without opt-in/out and probably not in the most efficient way.

It works on paths only; symlinks to files are left untouched.

This patch indeed makes the one from D7930 superfluous for me. For me it also triggers an apparently known issue where the parser gets confused and includes the wrong "debug.h" headers, leading to errors like Use of undeclared identifier 'CMAKE'.

Test Plan

the test_path unittest succeeds but would probably need to check the result of canonicalisation.

At least 2 places that could be optimised:

  • the concatenation of the canonicalised file path and the file name in toCanonicalPath()
  • the canonicalisation of the result of a concatenation in Path::addPath().

In theory there should be no need to make this feature optional; the patch emulates what happens at the OS level on Mac. One could consider providing a more or less visible option for advanced users to deactivate the feature if it breaks anything for them (or for optimal performance in contexts where canonicalisation is not required).

Diff Detail

Lint
Lint Skipped
Unit
Unit Tests Skipped
rjvbb created this revision.Sep 23 2017, 1:06 PM
rjvbb edited the summary of this revision. (Show Details)
rjvbb updated this revision to Diff 19840.Sep 23 2017, 9:27 PM

Print some debug information (if enabled).

rjvbb set the repository for this revision to R32 KDevelop.Sep 23 2017, 9:27 PM
mwolff requested changes to this revision.Sep 25 2017, 8:21 PM
mwolff added a subscriber: mwolff.

-2, I deliberately did not implement Path in this way and it should not be done. Users of Path should properly canonicalize the Paths. Feel free to add more asserts like those that exist in the DUChain to verify that paths are properly sanitized as-needed.

This revision now requires changes to proceed.Sep 25 2017, 8:21 PM
rjvbb added a comment.Sep 26 2017, 9:14 AM

Why push for downstreaming here, forcing users to duplicate a feature that could (should, IMHO) be provided by Path? Why would a method like the proposed one that generates the canonical path NOT be at its place in Path?

I didn't really mean to suggest that Path should always and exclusively work with canonical paths. Indeed it cannot know when it should canonicalise. Or rather, when it shouldn't. Maybe Path should indeed NOT convert the paths internally but only provide an option to canonicalise local paths on output.

R.

Because Path is a generic data structure. Doing it at the place where it's being used allows this feature to be handled in a much faster way for the common case. The UDSEntry from a KIO list job e.g. knows if something is a symlink already and we can then canonicalize the path as needed. Your patch would add the overhead of this duplicate check to every user.

I claim that we simply haven't yet fully understood why something breaks for you (cf. my last comment to D7930). As such, adding a workaround on such a low level is not the correct fix, it's simply a workaround.

rjvbb abandoned this revision.Sep 26 2017, 5:10 PM