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

[Greg Clayton]

> 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