[Lldb-commits] [PATCH] D53788: [FileSystem] Remove GetByteSize() from FileSpec

[Zachary Turner via Phabricator via lldb-commits]

zturner added a comment.

In https://reviews.llvm.org/D53788#1279096, @shafik wrote:

> Are we refactoring in the right place? Why not just refactor `FileSpec::GetByteSize()` why is `FileSpec` the wrong place?


There was another review thread where we discussed this just the other day.  Basically, It took a herculean effort to get `FileSpec` into the Utility library, which helped greatly with layering and we don't want to undo this.  In order to for `GetByteSize` to be able to use the VFS, it has to be able to use `FileSystem`, so either `FileSpec` has to move out of `Utility`, `FileSystem` has to move into `Utility`, or `GetByteSize` has to move out of `FileSpec`.  `FileSystem` moving into `Utility` might actually be a worthwhile goal long term, but even putting that aside, I think `FileSpec` should just be a thing for manipulating paths.  For example, it has the notion of manipulating paths which might not even make sense on the given host platform (e.g. it has a member variable which represents the path *syntax).  You might be on a Windows host debugging a remove Linux target, for example, and your host needs to have some way to manipulate paths on the remote which use a non-host syntax.  That's what `FileSpec` is supposed to be.  It just deals with strings.


Repository:
  rLLDB LLDB

https://reviews.llvm.org/D53788