[Lldb-commits] [PATCH] Skip symlinks to the original file when searching for debug info

[Zachary Turner]

Well I really like to keep ifdefs out of code.  I know that Host is the
place for ifdefs, and FileSpec is in Host, but I still like to keep them to
a minimum.  It's too easy to break things when you have to try to figure
out which ifdef paths you need to modify.  That was the reason for creating
the FileSystem class.  It exposes a platform agnostic interface to low
level FS operations.  Similar to llvm::sys::fs, and there's probably some
duplication there that could be removed, but I just didn't want to break
too much stuff at the time.

I really wouldn't want to see a bunch of ifdefs ending up in FileSpec
(there's already some there that I'd like to see removed, as described
below).  A lot of what it does is consistent across all platforms and is
just focused on splitting apart a string, and constifying the directory and
filename portions.  And so I think it would be a shame to muck up this
(mostly) platform-agnostic class with some platform specific stuff.  It
would be great if there were a way to isolate the disk access part.  I
don't think it's too crazy to write something like

FileSpec MyFile("/usr/bin/foo");
FileSystem::DirectoryEnumerator Enumerator(MyFile);
for (FileSpec &Child : Enumerator) {

  if (FileSystem::IsDirectory(Child)) {
      // Do something.
  }

}

In fact I think it's even better.

1. It improves readability by making it clear that you're hitting the file

system, so you should be careful about what type of path you're using.

2. It improves modularity by isolating the code that hits the file system

from the code that doesn't.

3. It cleans up the FileSpec implementation somewhat.  Currently

EnumerateDirectory is a big ugly function with a couple of different
preprocessor branches.

4. Separating the responsibilities like this leads to more flexible usage

patterns.  Like having DirectoryEnumerator be an object that can be used in
a range-based for loop here, instead of forcing its usage to conform to
what you can write with a single function call.

Anyway, TL;DR of all this is that I know it was originally designed to be
all things related to files, but as it grows, it starts to become really
heavy.  I think it makes sense to consider a separation of
responsibilities.  Does a function that mmaps file contents really belong
in the same class as a function that resolves a symlink?  I don't think we
need or even should have 1 class that is all things file system.  We should
try to find ways to break it down into smaller, more targeted, more
meaningful components.


http://reviews.llvm.org/D7836

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/