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/
_______________________________________________
lldb-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/lldb-commits
