Page MenuHomePhabricator

[2/2] Fix Path support on Windows
Needs ReviewPublic

Authored by zturner on Jul 25 2014, 11:52 AM.

Details

Reviewers
deepak2427
Summary

This gives FileSpec the notion of a path syntax, of which there are 3. Unix Native, Windows Native, and Host Native, which chooses between Unix and Windows automatically. This allows cross-platform support (e.g. storing a Unix Path on a Windows Host, and vice versa).

There are remaining Path issues in PathMappingList. These should be fixed independently by storing FileSpecs instead of ConstStrings, and using the fixes in this patch. Alternatively, llvm::sys::path contains path methods that might help here.

Diff Detail

Event Timeline

zturner updated this revision to Diff 11891.Jul 25 2014, 11:52 AM
zturner retitled this revision from to [2/2] Fix Path support on Windows.
zturner updated this object.
zturner edited the test plan for this revision. (Show Details)
zturner added a reviewer: deepak2427.
zturner added a subscriber: Unknown Object (MLST).
zturner updated this revision to Diff 11896.Jul 25 2014, 2:46 PM
zturner updated this object.

Fixed some issues related to normalization and de-normalization.

zturner updated this revision to Diff 11897.Jul 25 2014, 2:48 PM

Accidentally reversed the diff on the last upload.

zturner updated this revision to Diff 11898.Jul 25 2014, 2:52 PM

Removed dead function.

deepak2427 edited edge metadata.Jul 30 2014, 4:26 PM

Thanks for fixing this. I haven't had a chance to test this patch with our cross-platform issues yet, have to look at some other issues first.
Just a couple of doubts I had.

  • Are the paths always going to default as ePathSyntaxHostNative and then converted based on the Host?
  • In 733 llvm::SmallString<64> denormalized; , is the length of 64 enough?
  • I'm not that familiar with LLVM data structures, however would it be better to keep llvm::SmallString itself for the Normalize/Denormalize functions to keep it consistent?

Thanks for fixing this. I haven't had a chance to test this patch with our cross-platform issues yet, have to look at some other issues first.
Just a couple of doubts I had.

  • Are the paths always going to default as ePathSyntaxHostNative and then converted based on the Host?
  • In 733 llvm::SmallString<64> denormalized; , is the length of 64 enough?
  • I'm not that familiar with LLVM data structures, however would it be better to keep llvm::SmallString itself for the Normalize/Denormalize functions to keep it consistent?

Paths default to SyntaxHostNative because that makes the semantics post-patch equivalent to pre-patch. Normalized form is basically unix form, so if the syntax ends up as ePathSyntaxPosix (either because it was explicitly specified or because host was specified and your host is posix), then normalization and de-normalization are both no-ops.

SmallString<64> just means that it keeps 64 bytes on the stack, and if it grows longer than that, it will fallback to help allocation. It still supports arbitrarily long strings, but paths tend to be small, so that's why a small number was chosen.