[Lldb-commits] [lldb] r216398 - Change back all paths returns for lldb::PathType enumerations from HostInfo::GetLLDBPath() to return the directories in the FileSpec.m_directory field to match previous implementations. This change previously broke some path stuff in upstream branches.

[Zachary Turner]

On Tue, Aug 26, 2014 at 1:13 PM, Greg Clayton <gclayton at apple.com> wrote:

>
> > On Aug 26, 2014, at 12:39 PM, Zachary Turner <zturner at google.com> wrote:
> >
> > I guess I'm thinking of this as the standard unix basename / dirname
> paradigm.  Everyone understands this paradigm and there's no confusion
> about it.  And given that FileSpec already internally stores its components
> in this format, it would be a natural fit for m_filename and m_dirname to
> correspond to basename and dirname, respectively.  In fact, the entire
> class is already implemented with this assumption.  See, for example,
> SetFile() which can be used to set arbitrary paths, which could refer to
> directories, but which still splits the last component and stores it in
> m_filename.  Or AppendPathComponent(), which handles the case where
> m_filename is not null, hence assuming that it must refer to a directory.
> >
> > As for the places that broke as a result of my last change - certainly
> they need to be fixed, so "reverting to green" is reasonable until we can
> decide if there's a better fix.  However, I did run all tests, and none of
> them failed.  I don't think the new test which checks that GetFilename() is
> null is particularly useful, because it just tests an implementation detail
> of the class, and not a documented behavior of the class.  It should be
> possible to write a test against whatever failed, which probably would be a
> better test.
>
> There was no test and thus why I added one. But I do agree that the API is
> not consistent.
>
> >
> > In any case, back to my original question: Is there any object to
> changing FileSpec::GetDirectory() and FileSpec::GetFilename() to be
> GetBaseName() and GetDirName()?
>
> I would just change it to GetLastPathComponent() and and leave
> GetDirectory() alone or change it to GetContainingDirectory().
>
> > This would require changing the call sites of the places who previously
> broke as a result of my original change.  I don't actually know what broke
> since my test runs were all successful, though.
>
> The other issue with changing things is that we have internal branches
> that are based on this code that are constantly requiring merging due to
> all of these changes that have been going on. I appreciate all the changes,
> and changing things to be better organized is nice.
>
> >  If this is not appropriate, then I think we need to change
> AppendPathComponent(), SetFile(), and various other functions to stop
> allowing situations where m_directory + m_filename refers to a directory.
>
> A few core design things:
> - no setters can require any stat calls to determine if things are
> directories unless someone explicitly asks the FileSpec for this info
> - basename and directory must remain separate ConstString values
>
> The Host calls that got the directories were optimized for the use cases
> where client asks for directory X and then sets the basename via the
> GetFilename() accessor. I would prefer to maintain this if possible to
> avoid contaminating the constant string pool and we can do this via APIs.
>
> Greg
>

I will keep all this in mind, thanks.  As for the internal branches
requiring merging, I understand that is a pain point.  If it's any
consolation, one of my explicit goals with the Host refactoring is making
it so that it's organized in such a way that you working on your internal
branches will have less risk of breaking anything I'm working on, and vice
versa.   Of course, to reach that goal it has to get worse before it gets
better.  But I think that once I'm done, much of what I need to do in the
future will be able to take place in the source/Host/windows directory, and
thusly have no effect on any other platform.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/lldb-commits/attachments/20140826/80127504/attachment.html>