<div dir="ltr"><br><div class="gmail_extra"><br><br><div class="gmail_quote">On Tue, Aug 26, 2014 at 1:13 PM, Greg Clayton <span dir="ltr"><<a href="mailto:gclayton@apple.com" target="_blank">gclayton@apple.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class=""><br>
> On Aug 26, 2014, at 12:39 PM, Zachary Turner <<a href="mailto:zturner@google.com">zturner@google.com</a>> wrote:<br>
><br>
> 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.<br>
><br>
> 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.<br>
<br>
</div>There was no test and thus why I added one. But I do agree that the API is not consistent.<br>
<div class=""><br>
><br>
> In any case, back to my original question: Is there any object to changing FileSpec::GetDirectory() and FileSpec::GetFilename() to be GetBaseName() and GetDirName()?<br>
<br>
</div>I would just change it to GetLastPathComponent() and and leave GetDirectory() alone or change it to GetContainingDirectory().<br>
<div class=""><br>
> 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.<br>
<br>
</div>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.<br>
<div class=""><br>
> 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.<br>
<br>
</div>A few core design things:<br>
- no setters can require any stat calls to determine if things are directories unless someone explicitly asks the FileSpec for this info<br>
- basename and directory must remain separate ConstString values<br>
<br>
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.<br>
<span class="HOEnZb"><font color="#888888"><br>
Greg<br></font></span></blockquote><div><br></div><div>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.</div>
</div><br></div></div>