Page MenuHomePhabricator

File path comparisons should be case-insensitive on OS X
Needs ReviewPublic

Authored by stigger on May 6 2016, 6:08 PM.

Details

Reviewers
zturner
Summary
  • File path comparisons should be case-insensitive on OS X
  • Fixed TestBreakpointCaseSensitivity

Diff Detail

Event Timeline

stigger updated this revision to Diff 56486.May 6 2016, 6:08 PM
stigger retitled this revision from to File path comparisons should be case-insensitive on OS X.
stigger updated this object.
stigger added a reviewer: zturner.
stigger added a subscriber: lldb-commits.

I don't know if this is a safe change. Mac OS X filesystems like HFS Plus may be case-insensitive (but case preserving) or they may be case sensitive. Network filesystems mounted on a Mac may be case sensitive. Without doing some attribute check on the filesystem (I don't know what API can tell you this off-hand) where the FileSpec is, I don't think this is a good idea.

These are valid points, but they apply to Linux and Windows as well: nothing prevents you from mounting a case-insensitive FS on Linux or a case-sensitive network share on Windows. However, in most cases the file system is going to be case-sensitive on Linux and case-insensitive on Windows.

It is true, that HFS supports optional case-sensitivity, but by default it is not being used, and 99% of Mac users out there are on case-insensitive FS.

Of course, it would be nice to dynamically check case sensitivity for every path, but that should be done for all platforms and is kind of out of scope of the problem that I tried to solve here. Plus, it might affect the performance.

Agreed, filesystems are almost always case sensitive on mac/ios. I'd hate for anyone to make the assumption that it's always the case, though - I work with network filesystems that are case sensitive every day so I'm very aware of the possibility. :)

Friendly ping.

zturner resigned from this revision.Jun 2 2016, 8:38 PM
zturner removed a reviewer: zturner.

I like the approach taken here in general.

One thing that I wonder about, if we have the Triple, why do we even need the PathSyntax at all? In light of the fact that not all Posix filesystems are case sensitive, this calls into question the usefulness of the Path Syntax since it is both unnecessary and insufficient.

I do think that FileSpec should be completely independent of Host. Any host detection code should be in FileSystem.h and any remote detection code should be in Platform somewhere. Both of those are independent of this patch though.

How difficult would be it be to eliminate the PathSyntax enumeration entirely and update every user of PathSyntax to use a triple instead?

How difficult would be it be to eliminate the PathSyntax enumeration entirely and update every user of PathSyntax to use a triple instead?

I've attempted to do it and hit two issues which I'm not sure how to resolve:

  1. SymbolFilePDB.cpp:358 uses FileSpec with explicit ePathSyntaxWindows. It's possible to do something like llvm::Triple("unknown", "unknown", "windows"), but that kind of smells.
  2. HostInfoBase.cpp:63 has a bunch of FileSpecs with ePathSyntaxHostNative, but at that moment it is not yet possible to use HostInfo::GetArchitecture(), because it's being constructed.
labath added a subscriber: labath.Sep 5 2016, 4:04 AM
include/lldb/Host/FileSpec.h
712

I don't think the default-null parameter here is a good idea. This means some of your file-specs will be case-sensitive and some will not (depending on whether the the person who created the FileSpec remembered to pass in the triple).

If it were up to me, I wouldn't even make case-sensitivity a property of the file spec. I think it should be a property of the comparison between them. Otherwise, what are you going to do when someone asks you to compare a case-insensitive "/foo/bar" with a case-sensitive "/Foo/Bar" ?

stigger added inline comments.Sep 5 2016, 4:34 AM
include/lldb/Host/FileSpec.h
712

If we get rid of PathSyntax, then default-null is going to be an equivalent of ePathSyntaxHostNative, meaning "use host triple".

Otherwise, what are you going to do when someone asks you to compare a case-insensitive "/foo/bar" with a case-sensitive "/Foo/Bar" ?

That's already handled in FileSpec::Equal and FileSpec::Compare: the comparison is case-insensitive only when both operands are case-insensitive.