Page MenuHomePhabricator

Fix a couple of cornercases in FileSpec + tests

Authored by labath on Mar 10 2016, 7:03 AM.



This fixes a couple of corner cases in FileSpec, related to AppendPathComponent and
handling of root directory (/) file spec. I add a bunch of unit tests for the new behavior.

Summary of changes:

FileSpec("/bar").GetCString(): before "//bar", after "/bar".
FileSpec("/").CopyByAppendingPathComponent("bar").GetCString(): before "//bar", after "/bar".
FileSpec("C:", ePathSyntaxWindows).CopyByAppendingPathComponent("bar").GetCString(): before "C:/bar", after "C:\bar".

Event Timeline

labath updated this revision to Diff 50275.Mar 10 2016, 7:03 AM
labath retitled this revision from to Fix a couple of cornercases in FileSpec + tests.
labath updated this object.
labath added reviewers: clayborg, zturner.
labath added a subscriber: lldb-commits.
57 ↗(On Diff #50275)

I've also identified one more issue in windows path handling (GetDirectory() always returns posix-style paths), which I do not attempt to address right now.

labath updated this object.Mar 10 2016, 7:27 AM
clayborg accepted this revision.Mar 10 2016, 10:24 AM
clayborg edited edge metadata.

Looks good.

This revision is now accepted and ready to land.Mar 10 2016, 10:24 AM
zturner accepted this revision.Mar 10 2016, 10:25 AM
zturner edited edge metadata.
This revision was automatically updated to reflect the committed changes.

This actually broke a couple of unit tests on Windows. Did you try running
these tests on Windows?

Interesting... I didn't run the tests on windows. I was hoping they would run mostly the same on all platforms. I'll try to give them a spin now...

labath added inline comments.Mar 16 2016, 6:19 AM

So this returns F: on linux and F:\ on Windows. This happens because llvm::sys::path::parent_path does not recognize F: as a "root directory" on linux, and therefore treats it differently. I don't know which behavior is more "correct" (probably the windows one), but I think that this should be consistent, regardless of the platform the test is run on (my original motivation for writing this was the fact that i was getting wonky paths while attempting to write other unit tests). Unfortunately, I think that means getting rid of llvm's path processing library...

What do you make of that?

(I am going on holiday, so I cannot to anything about this now. if you want to have a clean test run in the mean time, I am fine commenting these checks out or something...)

Yea, because of the fact that we must support both syntaxes on both
platforms, LLVM library is out.

The whole motivation for introducing the path syntax is so that windows
paths behave as if on Windows even if on linux