<div dir="ltr">In other words, it seems to me like m_directory and m_filename are implementation details whose only purpose is to reduce memory usage. i.e. since they use ConstString, it uses the fact that a directory can have many children (i.e. a one to many relationship) and gets more cache hits on the string pool by removing the last component of the path. Internally, however, these fields are not necessarily treated as strict filenames and directory names, but external users of the class, due to the confusing names, do treat them as strict filenames and directory names. If we could remove this ambiguity, and just provide GetPath() and GetParent(), and remove the ability to manually assign to the implementation details of the class, I think it would make things less confusing and more robust.</div>
<div class="gmail_extra"><br><br><div class="gmail_quote">On Tue, Aug 26, 2014 at 11:20 AM, Zachary Turner <span dir="ltr"><<a href="mailto:zturner@google.com" target="_blank">zturner@google.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">I think what I'm getting at is that the interface to the class is just confusing. For example, AppendPathComponent(x) will append together m_directory, m_filename, and x, then put everything after the last slash into m_filename and everything else into m_directory. So this function's understanding of the way the class is represented is that "filename" is not actually necessarily a filename, but rather just everything after the last slash. Different parts of the class seem to have a different understanding of what things mean. I think a better design would be to just delete the methods GetFilename() and GetDirectory(), and leave GetPath() (which returns the full string) and additional method GetParent() which returns m_filename. If you really want to know what type of object it is then stat it. Incidentally, there's an enumeration in there that is supposed to represent what type of FileSystem object it is (filesystem, directory, socket, etc) and which is never used for anything.</div>
<div class="HOEnZb"><div class="h5">
<div class="gmail_extra"><br><br><div class="gmail_quote">On Tue, Aug 26, 2014 at 11:07 AM, <span dir="ltr"><<a href="mailto:jingham@apple.com" target="_blank">jingham@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><br>
> On Aug 25, 2014, at 7:05 PM, Zachary Turner <<a href="mailto:zturner@google.com" target="_blank">zturner@google.com</a>> wrote:<br>
><br>
> It's possible I'm misunderstanding how this class works, or is supposed to work. Either way, its interface is confusing as written, so perhaps it needs to be re-thought.<br>
><br>
> But, if you call FileSpec::SetFile(foo), then my understanding is that foo represents an arbitrary path. It might be a directory, it might be a file, doesn't matter. FileSpec internally stores everything except the last component as the "directory", and the last component as the "file", even if directory+file actually points to a directory.<br>
><br>
> Is this not how it's supposed to work? I think it's confusing and inconsistent if that's not how it works. In other words, a multi-component "directory" field and a null path should be an invalid FileSpec in my mind. If you want to get back the entire thing, don't use file_spec.GetDirectory(), just use file_spec.GetPath(), that's what it's for.<br>
><br>
> Does this make sense? Thoughts?<br>
<br>
</div>But in this reading, FileSpec::GetFilename() returns that last directory component, doesn't it? That seems odd to me. I would expect GetFilename of a directory to return an empty string, since it isn't a file.<br>
<span><font color="#888888"><br>
Jim<br>
</font></span><div><div><br>
<br>
><br>
><br>
> On Mon, Aug 25, 2014 at 11:21 AM, Greg Clayton <<a href="mailto:gclayton@apple.com" target="_blank">gclayton@apple.com</a>> wrote:<br>
> Author: gclayton<br>
> Date: Mon Aug 25 13:21:06 2014<br>
> New Revision: 216398<br>
><br>
> URL: <a href="http://llvm.org/viewvc/llvm-project?rev=216398&view=rev" target="_blank">http://llvm.org/viewvc/llvm-project?rev=216398&view=rev</a><br>
> Log:<br>
> 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.<br>
><br>
><br>
> Modified:<br>
> lldb/trunk/source/Host/common/HostInfoBase.cpp<br>
> lldb/trunk/source/Host/linux/HostInfoLinux.cpp<br>
> lldb/trunk/source/Host/macosx/HostInfoMacOSX.mm<br>
> lldb/trunk/source/Host/posix/HostInfoPosix.cpp<br>
> lldb/trunk/source/Host/windows/HostInfoWindows.cpp<br>
><br>
> Modified: lldb/trunk/source/Host/common/HostInfoBase.cpp<br>
> URL: <a href="http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Host/common/HostInfoBase.cpp?rev=216398&r1=216397&r2=216398&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Host/common/HostInfoBase.cpp?rev=216398&r1=216397&r2=216398&view=diff</a><br>
> ==============================================================================<br>
> --- lldb/trunk/source/Host/common/HostInfoBase.cpp (original)<br>
> +++ lldb/trunk/source/Host/common/HostInfoBase.cpp Mon Aug 25 13:21:06 2014<br>
> @@ -228,7 +228,7 @@ HostInfoBase::ComputeSharedLibraryDirect<br>
> Host::GetModuleFileSpecForHostAddress(reinterpret_cast<void *>(reinterpret_cast<intptr_t>(HostInfoBase::GetLLDBPath))));<br>
><br>
> // Remove the filename so that this FileSpec only represents the directory.<br>
> - file_spec.SetFile(lldb_file_spec.GetDirectory().AsCString(), true);<br>
> + file_spec.GetDirectory() = lldb_file_spec.GetDirectory();<br>
><br>
> return (bool)file_spec.GetDirectory();<br>
> }<br>
> @@ -264,7 +264,7 @@ HostInfoBase::ComputeTempFileDirectory(F<br>
> // Make an atexit handler to clean up the process specify LLDB temp dir<br>
> // and all of its contents.<br>
> ::atexit(CleanupProcessSpecificLLDBTempDir);<br>
> - file_spec.SetFile(pid_tmpdir.GetString().c_str(), false);<br>
> + file_spec.GetDirectory().SetCStringWithLength(pid_tmpdir.GetString().c_str(), pid_tmpdir.GetString().size());<br>
> return true;<br>
> }<br>
><br>
><br>
> Modified: lldb/trunk/source/Host/linux/HostInfoLinux.cpp<br>
> URL: <a href="http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Host/linux/HostInfoLinux.cpp?rev=216398&r1=216397&r2=216398&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Host/linux/HostInfoLinux.cpp?rev=216398&r1=216397&r2=216398&view=diff</a><br>
> ==============================================================================<br>
> --- lldb/trunk/source/Host/linux/HostInfoLinux.cpp (original)<br>
> +++ lldb/trunk/source/Host/linux/HostInfoLinux.cpp Mon Aug 25 13:21:06 2014<br>
> @@ -194,7 +194,8 @@ HostInfoLinux::GetProgramFileSpec()<br>
> bool<br>
> HostInfoLinux::ComputeSystemPluginsDirectory(FileSpec &file_spec)<br>
> {<br>
> - file_spec.SetFile("/usr/lib/lldb", true);<br>
> + FileSpec temp_file("/usr/lib/lldb", true);<br>
> + file_spec.GetDirectory().SetCString(temp_file.GetPath().c_str());<br>
> return true;<br>
> }<br>
><br>
> @@ -204,17 +205,15 @@ HostInfoLinux::ComputeUserPluginsDirecto<br>
> // XDG Base Directory Specification<br>
> // <a href="http://standards.freedesktop.org/basedir-spec/basedir-spec-latest.html" target="_blank">http://standards.freedesktop.org/basedir-spec/basedir-spec-latest.html</a><br>
> // If XDG_DATA_HOME exists, use that, otherwise use ~/.local/share/lldb.<br>
> - FileSpec lldb_file_spec;<br>
> const char *xdg_data_home = getenv("XDG_DATA_HOME");<br>
> if (xdg_data_home && xdg_data_home[0])<br>
> {<br>
> std::string user_plugin_dir(xdg_data_home);<br>
> user_plugin_dir += "/lldb";<br>
> - lldb_file_spec.SetFile(user_plugin_dir.c_str(), true);<br>
> + file_spec.GetDirectory().SetCString(user_plugin_dir.c_str());<br>
> }<br>
> else<br>
> - lldb_file_spec.SetFile("~/.local/share/lldb", true);<br>
> -<br>
> + file_spec.GetDirectory().SetCString("~/.local/share/lldb");<br>
> return true;<br>
> }<br>
><br>
><br>
> Modified: lldb/trunk/source/Host/macosx/HostInfoMacOSX.mm<br>
> URL: <a href="http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Host/macosx/HostInfoMacOSX.mm?rev=216398&r1=216397&r2=216398&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Host/macosx/HostInfoMacOSX.mm?rev=216398&r1=216397&r2=216398&view=diff</a><br>
> ==============================================================================<br>
> --- lldb/trunk/source/Host/macosx/HostInfoMacOSX.mm (original)<br>
> +++ lldb/trunk/source/Host/macosx/HostInfoMacOSX.mm Mon Aug 25 13:21:06 2014<br>
> @@ -149,7 +149,7 @@ HostInfoMacOSX::ComputeSupportExeDirecto<br>
> ::strncpy(framework_pos, "/Resources", PATH_MAX - (framework_pos - raw_path));<br>
> #endif<br>
> }<br>
> - file_spec.SetFile(raw_path, true);<br>
> + file_spec.GetDirectory().SetCString(raw_path);<br>
> return (bool)file_spec.GetDirectory();<br>
> }<br>
><br>
> @@ -169,7 +169,7 @@ HostInfoMacOSX::ComputeHeaderDirectory(F<br>
> framework_pos += strlen("LLDB.framework");<br>
> ::strncpy(framework_pos, "/Headers", PATH_MAX - (framework_pos - raw_path));<br>
> }<br>
> - file_spec.SetFile(raw_path, true);<br>
> + file_spec.GetDirectory().SetCString(raw_path);<br>
> return true;<br>
> }<br>
><br>
> @@ -199,7 +199,7 @@ HostInfoMacOSX::ComputePythonDirectory(F<br>
> // We may get our string truncated. Should we protect this with an assert?<br>
> ::strncat(raw_path, python_version_dir.c_str(), sizeof(raw_path) - strlen(raw_path) - 1);<br>
> }<br>
> - file_spec.SetFile(raw_path, true);<br>
> + file_spec.GetDirectory().SetCString(raw_path);<br>
> return true;<br>
> }<br>
><br>
> @@ -218,14 +218,15 @@ HostInfoMacOSX::ComputeSystemPluginsDire<br>
><br>
> framework_pos += strlen("LLDB.framework");<br>
> ::strncpy(framework_pos, "/Resources/PlugIns", PATH_MAX - (framework_pos - raw_path));<br>
> - file_spec.SetFile(raw_path, true);<br>
> + file_spec.GetDirectory().SetCString(raw_path);<br>
> return true;<br>
> }<br>
><br>
> bool<br>
> HostInfoMacOSX::ComputeUserPluginsDirectory(FileSpec &file_spec)<br>
> {<br>
> - file_spec.SetFile("~/Library/Application Support/LLDB/PlugIns", true);<br>
> + FileSpec temp_file("~/Library/Application Support/LLDB/PlugIns", true);<br>
> + file_spec.GetDirectory().SetCString(temp_file.GetPath().c_str());<br>
> return true;<br>
> }<br>
><br>
><br>
> Modified: lldb/trunk/source/Host/posix/HostInfoPosix.cpp<br>
> URL: <a href="http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Host/posix/HostInfoPosix.cpp?rev=216398&r1=216397&r2=216398&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Host/posix/HostInfoPosix.cpp?rev=216398&r1=216397&r2=216398&view=diff</a><br>
> ==============================================================================<br>
> --- lldb/trunk/source/Host/posix/HostInfoPosix.cpp (original)<br>
> +++ lldb/trunk/source/Host/posix/HostInfoPosix.cpp Mon Aug 25 13:21:06 2014<br>
> @@ -158,14 +158,15 @@ HostInfoPosix::ComputeSupportExeDirector<br>
> log->Printf("Host::%s() failed to find /lib/liblldb within the shared lib path, bailing on bin path construction",<br>
> __FUNCTION__);<br>
> }<br>
> - file_spec.SetFile(raw_path, true);<br>
> + file_spec.GetDirectory().SetCString(raw_path);<br>
> return (bool)file_spec.GetDirectory();<br>
> }<br>
><br>
> bool<br>
> HostInfoPosix::ComputeHeaderDirectory(FileSpec &file_spec)<br>
> {<br>
> - file_spec.SetFile("/opt/local/include/lldb", false);<br>
> + FileSpec temp_file("/opt/local/include/lldb", false);<br>
> + file_spec.GetDirectory().SetCString(temp_file.GetPath().c_str());<br>
> return true;<br>
> }<br>
><br>
> @@ -187,6 +188,6 @@ HostInfoPosix::ComputePythonDirectory(Fi<br>
> // We may get our string truncated. Should we protect this with an assert?<br>
> ::strncat(raw_path, python_version_dir.c_str(), sizeof(raw_path) - strlen(raw_path) - 1);<br>
><br>
> - file_spec.SetFile(raw_path, true);<br>
> + file_spec.GetDirectory().SetCString(raw_path);<br>
> return true;<br>
> }<br>
><br>
> Modified: lldb/trunk/source/Host/windows/HostInfoWindows.cpp<br>
> URL: <a href="http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Host/windows/HostInfoWindows.cpp?rev=216398&r1=216397&r2=216398&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Host/windows/HostInfoWindows.cpp?rev=216398&r1=216397&r2=216398&view=diff</a><br>
> ==============================================================================<br>
> --- lldb/trunk/source/Host/windows/HostInfoWindows.cpp (original)<br>
> +++ lldb/trunk/source/Host/windows/HostInfoWindows.cpp Mon Aug 25 13:21:06 2014<br>
> @@ -107,6 +107,6 @@ HostInfoWindows::ComputePythonDirectory(<br>
> lldb_file_spec.AppendPathComponent("../lib/site-packages");<br>
> lldb_file_spec.GetPath(raw_path, sizeof(raw_path));<br>
><br>
> - file_spec.SetFile(raw_path, true);<br>
> + file_spec.GetDirectory().SetCString(raw_path);<br>
> return true;<br>
> }<br>
><br>
><br>
> _______________________________________________<br>
> lldb-commits mailing list<br>
> <a href="mailto:lldb-commits@cs.uiuc.edu" target="_blank">lldb-commits@cs.uiuc.edu</a><br>
> <a href="http://lists.cs.uiuc.edu/mailman/listinfo/lldb-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/lldb-commits</a><br>
><br>
> _______________________________________________<br>
> lldb-commits mailing list<br>
> <a href="mailto:lldb-commits@cs.uiuc.edu" target="_blank">lldb-commits@cs.uiuc.edu</a><br>
> <a href="http://lists.cs.uiuc.edu/mailman/listinfo/lldb-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/lldb-commits</a><br>
<br>
</div></div></blockquote></div><br></div>
</div></div></blockquote></div><br></div>