This is an archive of the discontinued LLVM Phabricator instance.

FileSpec: make matching separator-agnostic again
ClosedPublic

Authored by labath on Apr 13 2016, 8:26 AM.

Details

Summary

In D18689, I removed the call to Normalize() in FileSpec::SetFile, because it no longer seemed
needed, and it resolved a quirk in the FileSpec API (spec.GetCString() returnes a path with
backslashes, but spec.GetDirectory().GetCString() has forward slashes). This turned out to be a
problem because we would consider paths with different separators as different (which led to
unresolved breakpoints for instance).

Here, I am putting back in the call to Normalize() and adding a unittest for FileSpec::Equal. I
am commenting out the GetDirectory unittests until we figure out the what is the expected
behaviour here.

Diff Detail

Repository
rL LLVM

Event Timeline

labath updated this revision to Diff 53567.Apr 13 2016, 8:26 AM
labath retitled this revision from to FileSpec: make matching separator-agnostic again.
labath updated this object.
labath added a reviewer: zturner.
labath added a subscriber: lldb-commits.
zturner edited edge metadata.Apr 13 2016, 8:31 AM
zturner added a subscriber: zturner.

The expected behavior imo is to normalize before returning in
GetDirectory(). Perhaps you could add a boolean with default value true?

It's a bit more complicated than it seems. GetDirectory() returns a reference into the FileSpec, and a lot of code modifies it through it. Changing that would require fixups in all of the users, which I'd want to do separately.

But I do agree that this is the best way forward.

Ahh that's unfortunate. I guess the thing to do is make SetFile and
SetDirectory. At least by changing the return type we can get the compiler
to tell us everywhere this is happening. Did you run the test suite on
Windows to make sure this doesnt break anything?

Ahh that's unfortunate. I guess the thing to do is make SetFile and
SetDirectory. At least by changing the return type we can get the compiler
to tell us everywhere this is happening. Did you run the test suite on
Windows to make sure this doesnt break anything?

Yeah, I had something like that in mind.

I didn't run the windows test suite, but I'd be very surprised if it broke anything, since I'm basically just restoring the previous behavior (before I started messing with this function, we were always running the normalization).

Ok sounds good

This revision was automatically updated to reflect the committed changes.