<div dir="ltr">Good catch, thanks!</div><br><div class="gmail_quote"><div dir="ltr">On Wed, Jun 13, 2018 at 9:34 AM Pavel Labath <<a href="mailto:labath@google.com">labath@google.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">On Wed, 13 Jun 2018 at 17:27, Jonas Devlieghere via lldb-commits<br>
<<a href="mailto:lldb-commits@lists.llvm.org" target="_blank">lldb-commits@lists.llvm.org</a>> wrote:<br>
><br>
> Author: jdevlieghere<br>
> Date: Wed Jun 13 09:23:21 2018<br>
> New Revision: 334615<br>
><br>
> URL: <a href="http://llvm.org/viewvc/llvm-project?rev=334615&view=rev" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project?rev=334615&view=rev</a><br>
> Log:<br>
> [FileSpec] Delegate common operations to llvm::sys::path<br>
><br>
> With the recent changes in FileSpec to use LLVM's path style, it is<br>
> possible to delegate a bunch of common path operations to LLVM's path<br>
> helpers. This means we only have to maintain a single implementation and<br>
> at the same time can benefit from the efforts made by the rest of the<br>
> LLVM community.<br>
><br>
> This is part one of a set of patches. There was no obvious way to split<br>
> this so I just worked from top to bottom.<br>
><br>
> Differential revision: <a href="https://reviews.llvm.org/D48084" rel="noreferrer" target="_blank">https://reviews.llvm.org/D48084</a><br>
><br>
> Modified:<br>
> lldb/trunk/include/lldb/Utility/FileSpec.h<br>
> lldb/trunk/source/Core/Debugger.cpp<br>
> lldb/trunk/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp<br>
> lldb/trunk/source/Plugins/Platform/Android/PlatformAndroid.cpp<br>
> lldb/trunk/source/Plugins/Platform/MacOSX/PlatformDarwinKernel.cpp<br>
> lldb/trunk/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp<br>
> lldb/trunk/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp<br>
> lldb/trunk/source/Utility/FileSpec.cpp<br>
> lldb/trunk/unittests/Utility/FileSpecTest.cpp<br>
><br>
> Modified: lldb/trunk/include/lldb/Utility/FileSpec.h<br>
> URL: <a href="http://llvm.org/viewvc/llvm-project/lldb/trunk/include/lldb/Utility/FileSpec.h?rev=334615&r1=334614&r2=334615&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/lldb/trunk/include/lldb/Utility/FileSpec.h?rev=334615&r1=334614&r2=334615&view=diff</a><br>
> ==============================================================================<br>
> --- lldb/trunk/include/lldb/Utility/FileSpec.h (original)<br>
> +++ lldb/trunk/include/lldb/Utility/FileSpec.h Wed Jun 13 09:23:21 2018<br>
> @@ -19,7 +19,7 @@<br>
> // Project includes<br>
> #include "lldb/Utility/ConstString.h"<br>
><br>
> -#include "llvm/ADT/StringRef.h" // for StringRef<br>
> +#include "llvm/ADT/StringRef.h"<br>
> #include "llvm/Support/FileSystem.h"<br>
> #include "llvm/Support/FormatVariadic.h"<br>
> #include "llvm/Support/Path.h"<br>
> @@ -605,6 +605,6 @@ template <> struct format_provider<lldb_<br>
> static void format(const lldb_private::FileSpec &F, llvm::raw_ostream &Stream,<br>
> StringRef Style);<br>
> };<br>
> -}<br>
> +} // namespace llvm<br>
><br>
> #endif // liblldb_FileSpec_h_<br>
><br>
> Modified: lldb/trunk/source/Core/Debugger.cpp<br>
> URL: <a href="http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Core/Debugger.cpp?rev=334615&r1=334614&r2=334615&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Core/Debugger.cpp?rev=334615&r1=334614&r2=334615&view=diff</a><br>
> ==============================================================================<br>
> --- lldb/trunk/source/Core/Debugger.cpp (original)<br>
> +++ lldb/trunk/source/Core/Debugger.cpp Wed Jun 13 09:23:21 2018<br>
> @@ -605,8 +605,8 @@ LoadPluginCallback(void *baton, llvm::sy<br>
> const FileSpec &file_spec) {<br>
> Status error;<br>
><br>
> - static ConstString g_dylibext("dylib");<br>
> - static ConstString g_solibext("so");<br>
> + static ConstString g_dylibext(".dylib");<br>
> + static ConstString g_solibext(".so");<br>
><br>
> if (!baton)<br>
> return FileSpec::eEnumerateDirectoryResultQuit;<br>
><br>
> Modified: lldb/trunk/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp<br>
> URL: <a href="http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp?rev=334615&r1=334614&r2=334615&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp?rev=334615&r1=334614&r2=334615&view=diff</a><br>
> ==============================================================================<br>
> --- lldb/trunk/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp (original)<br>
> +++ lldb/trunk/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp Wed Jun 13 09:23:21 2018<br>
> @@ -2047,8 +2047,8 @@ unsigned ObjectFileELF::ParseSymbols(Sym<br>
> // custom extension and file name makes it highly unlikely that this will<br>
> // collide with anything else.<br>
> ConstString file_extension = m_file.GetFileNameExtension();<br>
> - bool skip_oatdata_oatexec = file_extension == ConstString("oat") ||<br>
> - file_extension == ConstString("odex");<br>
> + bool skip_oatdata_oatexec = file_extension == ConstString(".oat") ||<br>
> + file_extension == ConstString(".odex");<br>
><br>
> ArchSpec arch;<br>
> GetArchitecture(arch);<br>
><br>
> Modified: lldb/trunk/source/Plugins/Platform/Android/PlatformAndroid.cpp<br>
> URL: <a href="http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Plugins/Platform/Android/PlatformAndroid.cpp?rev=334615&r1=334614&r2=334615&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Plugins/Platform/Android/PlatformAndroid.cpp?rev=334615&r1=334614&r2=334615&view=diff</a><br>
> ==============================================================================<br>
> --- lldb/trunk/source/Plugins/Platform/Android/PlatformAndroid.cpp (original)<br>
> +++ lldb/trunk/source/Plugins/Platform/Android/PlatformAndroid.cpp Wed Jun 13 09:23:21 2018<br>
> @@ -305,7 +305,7 @@ Status PlatformAndroid::DownloadSymbolFi<br>
> const FileSpec &dst_file_spec) {<br>
> // For oat file we can try to fetch additional debug info from the device<br>
> ConstString extension = module_sp->GetFileSpec().GetFileNameExtension();<br>
> - if (extension != ConstString("oat") && extension != ConstString("odex"))<br>
> + if (extension != ConstString(".oat") && extension != ConstString(".odex"))<br>
> return Status(<br>
> "Symbol file downloading only supported for oat and odex files");<br>
><br>
><br>
> Modified: lldb/trunk/source/Plugins/Platform/MacOSX/PlatformDarwinKernel.cpp<br>
> URL: <a href="http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Plugins/Platform/MacOSX/PlatformDarwinKernel.cpp?rev=334615&r1=334614&r2=334615&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Plugins/Platform/MacOSX/PlatformDarwinKernel.cpp?rev=334615&r1=334614&r2=334615&view=diff</a><br>
> ==============================================================================<br>
> --- lldb/trunk/source/Plugins/Platform/MacOSX/PlatformDarwinKernel.cpp (original)<br>
> +++ lldb/trunk/source/Plugins/Platform/MacOSX/PlatformDarwinKernel.cpp Wed Jun 13 09:23:21 2018<br>
> @@ -431,8 +431,8 @@ void PlatformDarwinKernel::AddSDKSubdirs<br>
> FileSpec::EnumerateDirectoryResult<br>
> PlatformDarwinKernel::FindKDKandSDKDirectoriesInDirectory(<br>
> void *baton, llvm::sys::fs::file_type ft, const FileSpec &file_spec) {<br>
> - static ConstString g_sdk_suffix = ConstString("sdk");<br>
> - static ConstString g_kdk_suffix = ConstString("kdk");<br>
> + static ConstString g_sdk_suffix = ConstString(".sdk");<br>
> + static ConstString g_kdk_suffix = ConstString(".kdk");<br>
><br>
> PlatformDarwinKernel *thisp = (PlatformDarwinKernel *)baton;<br>
> if (ft == llvm::sys::fs::file_type::directory_file &&<br>
> @@ -492,8 +492,8 @@ FileSpec::EnumerateDirectoryResult<br>
> PlatformDarwinKernel::GetKernelsAndKextsInDirectoryHelper(<br>
> void *baton, llvm::sys::fs::file_type ft, const FileSpec &file_spec,<br>
> bool recurse) {<br>
> - static ConstString g_kext_suffix = ConstString("kext");<br>
> - static ConstString g_dsym_suffix = ConstString("dSYM");<br>
> + static ConstString g_kext_suffix = ConstString(".kext");<br>
> + static ConstString g_dsym_suffix = ConstString(".dSYM");<br>
> static ConstString g_bundle_suffix = ConstString("Bundle");<br>
> ConstString file_spec_extension = file_spec.GetFileNameExtension();<br>
><br>
><br>
> Modified: lldb/trunk/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp<br>
> URL: <a href="http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp?rev=334615&r1=334614&r2=334615&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp?rev=334615&r1=334614&r2=334615&view=diff</a><br>
> ==============================================================================<br>
> --- lldb/trunk/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp (original)<br>
> +++ lldb/trunk/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp Wed Jun 13 09:23:21 2018<br>
> @@ -2590,9 +2590,9 @@ bool ScriptInterpreterPython::LoadScript<br>
> // strip .py or .pyc extension<br>
> ConstString extension = target_file.GetFileNameExtension();<br>
> if (extension) {<br>
> - if (::strcmp(extension.GetCString(), "py") == 0)<br>
> + if (llvm::StringRef(extension.GetCString()) == ".py")<br>
> basename.resize(basename.length() - 3);<br>
> - else if (::strcmp(extension.GetCString(), "pyc") == 0)<br>
> + else if (llvm::StringRef(extension.GetCString()) == ".pyc")<br>
> basename.resize(basename.length() - 4);<br>
> }<br>
> } else {<br>
><br>
> Modified: lldb/trunk/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp<br>
> URL: <a href="http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp?rev=334615&r1=334614&r2=334615&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp?rev=334615&r1=334614&r2=334615&view=diff</a><br>
> ==============================================================================<br>
> --- lldb/trunk/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp (original)<br>
> +++ lldb/trunk/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp Wed Jun 13 09:23:21 2018<br>
> @@ -1653,7 +1653,7 @@ void SymbolFileDWARF::UpdateExternalModu<br>
> // (corresponding to .dwo) so we simply skip it.<br>
> if (m_obj_file->GetFileSpec()<br>
> .GetFileNameExtension()<br>
> - .GetStringRef() == "dwo" &&<br>
> + .GetStringRef() == ".dwo" &&<br>
> llvm::StringRef(m_obj_file->GetFileSpec().GetPath())<br>
> .endswith(dwo_module_spec.GetFileSpec().GetPath())) {<br>
> continue;<br>
><br>
> Modified: lldb/trunk/source/Utility/FileSpec.cpp<br>
> URL: <a href="http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Utility/FileSpec.cpp?rev=334615&r1=334614&r2=334615&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Utility/FileSpec.cpp?rev=334615&r1=334614&r2=334615&view=diff</a><br>
> ==============================================================================<br>
> --- lldb/trunk/source/Utility/FileSpec.cpp (original)<br>
> +++ lldb/trunk/source/Utility/FileSpec.cpp Wed Jun 13 09:23:21 2018<br>
> @@ -12,15 +12,15 @@<br>
> #include "lldb/Utility/Stream.h"<br>
> #include "lldb/Utility/TildeExpressionResolver.h"<br>
><br>
> -#include "llvm/ADT/SmallString.h" // for SmallString<br>
> -#include "llvm/ADT/SmallVector.h" // for SmallVectorTemplat...<br>
> +#include "llvm/ADT/SmallString.h"<br>
> +#include "llvm/ADT/SmallVector.h"<br>
> #include "llvm/ADT/StringRef.h"<br>
> -#include "llvm/ADT/Triple.h" // for Triple<br>
> -#include "llvm/ADT/Twine.h" // for Twine<br>
> -#include "llvm/Support/ErrorOr.h" // for ErrorOr<br>
> +#include "llvm/ADT/Triple.h"<br>
> +#include "llvm/ADT/Twine.h"<br>
> +#include "llvm/Support/ErrorOr.h"<br>
> #include "llvm/Support/FileSystem.h"<br>
> #include "llvm/Support/Program.h"<br>
> -#include "llvm/Support/raw_ostream.h" // for raw_ostream, fs<br>
> +#include "llvm/Support/raw_ostream.h"<br>
><br>
> #include <algorithm> // for replace, min, unique<br>
> #include <system_error> // for error_code<br>
> @@ -50,7 +50,7 @@ bool PathStyleIsPosix(FileSpec::Style st<br>
> }<br>
><br>
> const char *GetPathSeparators(FileSpec::Style style) {<br>
> - return PathStyleIsPosix(style) ? "/" : "\\/";<br>
> + return llvm::sys::path::get_separator(style).data();<br>
> }<br>
><br>
> char GetPreferredPathSeparator(FileSpec::Style style) {<br>
> @@ -67,30 +67,6 @@ void Denormalize(llvm::SmallVectorImpl<c<br>
><br>
> std::replace(path.begin(), path.end(), '/', '\\');<br>
> }<br>
> -<br>
> -bool PathIsRelative(llvm::StringRef path, FileSpec::Style style) {<br>
> -<br>
> - if (path.empty())<br>
> - return false;<br>
> -<br>
> - if (PathStyleIsPosix(style)) {<br>
> - // If the path doesn't start with '/' or '~', return true<br>
> - switch (path[0]) {<br>
> - case '/':<br>
> - case '~':<br>
> - return false;<br>
> - default:<br>
> - return true;<br>
> - }<br>
> - } else {<br>
> - if (path.size() >= 2 && path[1] == ':')<br>
> - return false;<br>
> - if (path[0] == '/')<br>
> - return false;<br>
> - return true;<br>
> - }<br>
> - return false;<br>
> -}<br>
><br>
> } // end anonymous namespace<br>
><br>
> @@ -239,7 +215,7 @@ bool needsNormalization(const llvm::Stri<br>
> return false;<br>
> }<br>
><br>
> -<br>
> +<br>
> }<br>
> //------------------------------------------------------------------<br>
> // Assignment operator.<br>
> @@ -286,15 +262,19 @@ void FileSpec::SetFile(llvm::StringRef p<br>
> if (resolved.empty()) {<br>
> // If we have no path after normalization set the path to the current<br>
> // directory. This matches what python does and also a few other path<br>
> - // utilities.<br>
> + // utilities.<br>
> m_filename.SetString(".");<br>
> return;<br>
> }<br>
><br>
> - m_filename.SetString(llvm::sys::path::filename(resolved, m_style));<br>
> - llvm::StringRef dir = llvm::sys::path::parent_path(resolved, m_style);<br>
> - if (!dir.empty())<br>
> - m_directory.SetString(dir);<br>
> + // Split path into filename and directory. We rely on the underlying char<br>
> + // pointer to be nullptr when the components are empty.<br>
> + llvm::StringRef filename = llvm::sys::path::filename(resolved, m_style);<br>
> + if(!filename.empty())<br>
> + m_filename.SetString(filename);<br>
> + llvm::StringRef directory = llvm::sys::path::parent_path(resolved, m_style);<br>
> + if(!directory.empty())<br>
> + m_directory.SetString(directory);<br>
> }<br>
><br>
> void FileSpec::SetFile(llvm::StringRef path, bool resolve,<br>
> @@ -441,16 +421,15 @@ int FileSpec::Compare(const FileSpec &a,<br>
> }<br>
><br>
> bool FileSpec::Equal(const FileSpec &a, const FileSpec &b, bool full) {<br>
> -<br>
> // case sensitivity of equality test<br>
> const bool case_sensitive = a.IsCaseSensitive() || b.IsCaseSensitive();<br>
> -<br>
> +<br>
> const bool filenames_equal = ConstString::Equals(a.m_filename,<br>
> b.m_filename,<br>
> case_sensitive);<br>
><br>
> if (!filenames_equal)<br>
> - return false;<br>
> + return false;<br>
><br>
> if (!full && (a.GetDirectory().IsEmpty() || b.GetDirectory().IsEmpty()))<br>
> return filenames_equal;<br>
> @@ -523,7 +502,7 @@ bool FileSpec::ResolvePath() {<br>
> return true; // We have already resolved this path<br>
><br>
> // SetFile(...) will set m_is_resolved correctly if it can resolve the path<br>
> - SetFile(GetPath(false), true);<br>
> + SetFile(GetPath(false), true, m_style);<br>
> return m_is_resolved;<br>
> }<br>
><br>
> @@ -606,25 +585,17 @@ void FileSpec::GetPath(llvm::SmallVector<br>
> }<br>
><br>
> ConstString FileSpec::GetFileNameExtension() const {<br>
> - if (m_filename) {<br>
> - const char *filename = m_filename.GetCString();<br>
> - const char *dot_pos = strrchr(filename, '.');<br>
> - if (dot_pos && dot_pos[1] != '\0')<br>
> - return ConstString(dot_pos + 1);<br>
> - }<br>
> - return ConstString();<br>
> + llvm::SmallString<64> current_path;<br>
> + current_path.append(m_filename.GetStringRef().begin(),<br>
> + m_filename.GetStringRef().end());<br>
> + return ConstString(llvm::sys::path::extension(current_path, m_style));<br>
> }<br>
<br>
Why do you need the temporary current_path here? It should be possible<br>
to pass the m_filename.GetStringRef() straight to<br>
llvm::sys::path::extension..<br>
</blockquote></div>