> On Feb 15, 2017, at 10:14 AM, Zachary Turner <ztur...@google.com> wrote: > > I think the main improvement that it provides is that you need *something* > which represents only a path. Because we use FileSpecs to refer to paths on > remote machines, and then what does it mean to call Exists or Resolve? So, > we could have something like PathSpec and then have FileSpec contain a > PathSpec, and PathSpec's only purpose would be to know about the grammar of a > path.
Agreed. Are you ok with starting with moving as much stuff from FileSpec over into File to start with? > > I agree though, in the spirit of doing things incrementally, purely moving > things around and not making major substantive changes is a good starting > point and reduces the scope of the problem. Agreed. > > On Wed, Feb 15, 2017 at 10:07 AM Greg Clayton <gclay...@apple.com > <mailto:gclay...@apple.com>> wrote: > I would prefer things stay object oriented when possible. I don't think it > adds anything by making everything a static function. It is ok where it makes > sense, but I would vote to add static functions in the FileSpec class over > making purely stand alone functions as they indicate that it should be used > on a FileSpec object. It is also ok to have both methods and static functions > where the methods (like AppendPathComponent() can call through to the static > versions if needed. > > We also have the "File.h" class that defines lldb_private::File. This is the > object that wraps the access to the data in the file (the file descriptor > integers and the "FILE *" abstraction. It would be interesting to move any > functions in lldb_private::FileSpec, like the mmap functions, over into > lldb_private::File where needed. And then see if there is even a need for the > FileSystem class at all. If there is we can divvy it up as needed. > > I would say lets start with purely moving things around and get the file > locations all set and the correct abstractions where FileSpec is for > representing a file specification efficiently and all operations on that > affect file spec mutations, moving anything that is accessing file contents > over into lldb_private::File, and then seeing if we have any functions left > over and if we even need FileSystem. > > That being said I generally find it useful to do something like: > > FileSpec f("/tmp/a.txt"); > if (f.Exists()) > ... > > > It seems like the suggestions would be to move to something like: > > FileSpec f("/tmp/a.txt"); > if (File::Exists(f)) > ... > > Both are fine, but the latter is much less discoverable. Think about when you > use IDEs with auto completion. So while it might be cleaner from a code > linking standpoint, I don't think it actually improves the code for users. I > much prefer the object oriented methodology and I would prefer code linking > considerations don't override the clean API we currently have. > > > On Feb 15, 2017, at 8:05 AM, Pavel Labath via lldb-dev > > <lldb-dev@lists.llvm.org <mailto:lldb-dev@lists.llvm.org>> wrote: > > > > I prefer free functions as well, but I think switching FileSpec to > > that would be a non-trivial task. If you want to try it out I would > > encourage it, but I don't think it's a requirement. > > > > On 15 February 2017 at 15:41, Zachary Turner <ztur...@google.com > > <mailto:ztur...@google.com>> wrote: > >> Having FileSpec in Utility and FileSystem in Host would work for now. > >> > >> Any opinions on the FileSpec interface? Llvm's path and file system > >> libraries use free functions for everything. in a perfect world I feel > >> that's the best design, and with lldb we could even do slightly better and > >> make all functions pure (returning copies instead of mutating) since > >> everything is ConstString. > >> > >> If we were starting over from scratch this would definitely be the way to > >> go > >> imo, but I'm not sure if it's worth the effort now. What do you think? > >> > >> On Wed, Feb 15, 2017 at 7:34 AM Pavel Labath <lab...@google.com > >> <mailto:lab...@google.com>> wrote: > >>> > >>> Right, I see. In that case, I guess my response would be "let's wait > >>> and see how things look like after FileSpec is moved". > >>> > >>> I kinda like how we have the all the host code in a separate module (I > >>> expect we will have a lot more of it than llvm), but I am not against > >>> it if it turns out there is no other way to organize dependencies. I > >>> just don't think we've reached that point yet -- right now I don't see > >>> why we couldn't have FileSpec in Utility and FileSystem in Host. > >>> > >>> Or have you thought ahead and found a problem already? > >>> > >>> > >>> On 15 February 2017 at 15:17, Zachary Turner <ztur...@google.com > >>> <mailto:ztur...@google.com>> wrote: > >>>> Yes, in fact that mirrors how i had planned to tackle this. > >>>> > >>>> The question is, can we put in Utility code that is separated by > >>>> platform, > >>>> which typically has been for Host? This mirrors what llvm does > >>>> On Wed, Feb 15, 2017 at 6:29 AM Pavel Labath <lab...@google.com > >>>> <mailto:lab...@google.com>> wrote: > >>>>> > >>>>> I agree that the next module that needs figuring on is the host one. > >>>>> However I am not sure if the decision on what to put in what module > >>>>> should be motivated by the FileSpec class, as I think it's current > >>>>> interface has a some issues, and our choice on how to resolve them can > >>>>> greatly affect what things it depends on. > >>>>> > >>>>> The main thing that bugs me with FileSpec is that it is used for a two > >>>>> distinct purposes: > >>>>> - manipulations on abstract path names (e.g. PrependPathComponent) > >>>>> - manipulations of actual files present on the host (e.g. > >>>>> MemoryMapFileContents) > >>>>> For the first one you don't need the files to exist on the host (in > >>>>> fact they may not even use the same path syntax as the host). For the > >>>>> other one, they *must* exist and *must* use the same path syntax. This > >>>>> is currently not very well separated and enforced, and I think it > >>>>> should be. I believe this is the reason the FileSystem pseudo-class > >>>>> was created. > >>>>> > >>>>> So, my counter-proposal would be to finish moving the host-specific > >>>>> things out of the FileSpec class (basically just replace > >>>>> file_spec.Foo(...) with FileSystem::Foo(file_spec, ...). At that point > >>>>> FileSpec should only depend on string manipulation functions, and we > >>>>> should be able to move it without much hassle. After that, we can take > >>>>> another look and decide what to do next. > >>>>> > >>>>> The thing I really like about this idea is that we will end up with > >>>>> two classes that very closely mirror llvm functionality (FileSpec is a > >>>>> version of Support/Path.h that does not assume host path syntax, > >>>>> FileSystem is similar to Support/FileSystem.h, but it supports some > >>>>> more fancy operations like mmap()). Then we could proceed to merge > >>>>> this functionality with llvm pretty much independently of any other > >>>>> refactoring we will be doing. > >>>>> > >>>>> What do you think? > >>>>> > >>>>> > >>>>> > >>>>> On 15 February 2017 at 01:48, Zachary Turner via lldb-dev > >>>>> <lldb-dev@lists.llvm.org <mailto:lldb-dev@lists.llvm.org>> wrote: > >>>>>> After https://reviews.llvm.org/D29964 > >>>>>> <https://reviews.llvm.org/D29964>, we finally have a starting > >>>>>> point > >>>>>> at > >>>>>> which we can begin unravelling the cross-project cyclic dependencies > >>>>>> in > >>>>>> LLDB. lldb/Utility now is very similar in spirit to llvm/Support. > >>>>>> > >>>>>> But llvmSupport goes one step further and includes what lldb would > >>>>>> normally > >>>>>> put under Host. I think this makes some sense. Practically all > >>>>>> parts > >>>>>> of a > >>>>>> codebase have need of a single OS abstraction layer. So, I think > >>>>>> that a > >>>>>> lot > >>>>>> of the functionality currently in lldbHost is in fact needed by the > >>>>>> rest > >>>>>> of > >>>>>> LLDB. > >>>>>> > >>>>>> So, I wonder if it makes sense to follow the path that LLVM has > >>>>>> taken, > >>>>>> and > >>>>>> start moving some of this code from Host down to Utility. Doing so > >>>>>> would > >>>>>> allow us to break one of the biggest links in the dependency cycle in > >>>>>> the > >>>>>> entire codebase, which is that Host depends on everything, and > >>>>>> everything > >>>>>> depends on Host. > >>>>>> > >>>>>> Of course, it can't just be a straight move. Some things in Host > >>>>>> interact > >>>>>> with Target, with CommandInterpreter, and with many other things. > >>>>>> And > >>>>>> stuff > >>>>>> going into Utility can't take a dependency. > >>>>>> > >>>>>> So there will be some splitting, some moving, some refactoring, etc. > >>>>>> But to > >>>>>> me tackling Host seems like the logical next step, in large part > >>>>>> because > >>>>>> Host is where FileSpec is, and it's going to be hard to break any > >>>>>> dependencies without first addressing FileSpec. > >>>>>> > >>>>>> The way LLVM handles cross-platform differences in Support is that > >>>>>> you > >>>>>> include a single header and it does conditional includes into a > >>>>>> platform > >>>>>> specific subdirectory for the parts that differ. > >>>>>> > >>>>>> I'm thinking to follow the same pattern here in lldb/Utility, and > >>>>>> begin > >>>>>> looking for ways to get pieces of Host down into Utility this way, > >>>>>> until > >>>>>> ultimately I can get FileSpec down there. > >>>>>> > >>>>>> Thoughts? > >>>>>> > >>>>>> _______________________________________________ > >>>>>> lldb-dev mailing list > >>>>>> lldb-dev@lists.llvm.org <mailto:lldb-dev@lists.llvm.org> > >>>>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-dev > >>>>>> <http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-dev> > >>>>>> > > _______________________________________________ > > lldb-dev mailing list > > lldb-dev@lists.llvm.org <mailto:lldb-dev@lists.llvm.org> > > http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-dev > > <http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-dev> >
_______________________________________________ lldb-dev mailing list lldb-dev@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-dev
