[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.

On Mon Feb 23 2015 at 1:23:54 PM <jingham at apple.com> wrote:

>
> > On Feb 23, 2015, at 1:17 PM, Zachary Turner <zturner at google.com> wrote:
> >
> > In http://reviews.llvm.org/D7836#128511, @jingham wrote:
> >
> >> That is not how we thought of FileSpec originally.  It clearly was
> meant to deal with real files (for instance it has Exists methods & Read,
> etc.)  It was meant to be a wrapper that would allow us to do all the
> useful file manipulation in a system-independent way.  It ended up with
> system dependencies just because we didn't have the time to keep it clean
> when there were no ports of lldb that weren't Unix...  But that's life, not
> design.
> >>
> >> Anyway, it would be weird to have some object you use to denote the
> files you are interested in, but then when you actually want to do
> something with them, you have to turn around and use another object.  And
> it would be unfortunate to have to write system specific code at this point
> except for the - hopefully very few - cases where the sort of thing we want
> to do to a file is not able to be expressed in a system-independent way.
> >>
> >> Jim
> >
> >
> > Well, already we have introduced the notion of the path syntax so that
> FileSpec can in theory refer to a path that is invalid for the host you're
> on.  In that case it doesn't make much sense to have a Windows path, for
> example on Mac, and then try to do a file system operation on it.  When I
> refactored some of Host months ago, there were other methods directly in
> Host that would do things like get file permissions, etc.  So there was
> already some separation between the FileSpec and the methods that hit the
> file system.  Most of that stuff ended up in FileSystem, and the stuff that
> remains in FileSpec is only there because I couldn't change the public API.
> >
> > But with the notion of a FileSpec that can refer to a path that can't
> possibly be on your local host, I think it makes sense to consider whether
> the file system access routines should be separated from FileSpec.
>
> Not sure, wouldn't it be better to optionally attach a platform to
> FileSpec, so that FileSpecs can transparently handle remote access?
> FileSpec's are how we specify files. Seems awkward to switch gears to
> something else when you actually want to access them.
>
> Jim
>
>
> >
> >
> > http://reviews.llvm.org/D7836
> >
> > EMAIL PREFERENCES
> >  http://reviews.llvm.org/settings/panel/emailpreferences/
> >
> >
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/lldb-commits/attachments/20150223/be9af474/attachment.html>