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

[Zachary Turner]

Bleh: This came across really ugly in the email Phab sent out due to me responding from email.  Not sure if this will be any be better, but here it is again

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/