Re: [Lldb-commits] [lldb] r335432 - [FileSpec] Always normalize

[Pavel Labath via lldb-commits]

I believe the raison d'être of the needsNormalization function was the
normalization process is not exactly cheap (it constructs a vector of
StringRefs and whatnot), and measurements showed that this actually is
important for performance of lldb
<https://reviews.llvm.org/D45977#1078510>.
On Sun, 24 Jun 2018 at 14:36, Jonas Devlieghere via lldb-commits
<lldb-commits@lists.llvm.org> wrote:
>
> Author: jdevlieghere
> Date: Sun Jun 24 06:31:44 2018
> New Revision: 335432
>
> URL: http://llvm.org/viewvc/llvm-project?rev=335432&view=rev
> Log:
> [FileSpec] Always normalize
>
> Removing redundant components from the path seems pretty harmless.
> Rather than checking whether this is necessary and then actually doing
> so, always invoke remove_dots to start with a normalized path.
>
> Modified:
>     lldb/trunk/source/Utility/FileSpec.cpp
>
> Modified: lldb/trunk/source/Utility/FileSpec.cpp
> URL: 
> http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Utility/FileSpec.cpp?rev=335432&r1=335431&r2=335432&view=diff
> ==============================================================================
> --- lldb/trunk/source/Utility/FileSpec.cpp (original)
> +++ lldb/trunk/source/Utility/FileSpec.cpp Sun Jun 24 06:31:44 2018
> @@ -119,100 +119,6 @@ FileSpec::FileSpec(const FileSpec *rhs)
>  //------------------------------------------------------------------
>  FileSpec::~FileSpec() {}
>
> -namespace {
> -//------------------------------------------------------------------
> -/// Safely get a character at the specified index.
> -///
> -/// @param[in] path
> -///     A full, partial, or relative path to a file.
> -///
> -/// @param[in] i
> -///     An index into path which may or may not be valid.
> -///
> -/// @return
> -///   The character at index \a i if the index is valid, or 0 if
> -///   the index is not valid.
> -//------------------------------------------------------------------
> -inline char safeCharAtIndex(const llvm::StringRef &path, size_t i) {
> -  if (i < path.size())
> -    return path[i];
> -  return 0;
> -}
> -
> -//------------------------------------------------------------------
> -/// Check if a path needs to be normalized.
> -///
> -/// Check if a path needs to be normalized. We currently consider a
> -/// path to need normalization if any of the following are true
> -///  - path contains "/./"
> -///  - path contains "/../"
> -///  - path contains "//"
> -///  - path ends with "/"
> -/// Paths that start with "./" or with "../" are not considered to
> -/// need normalization since we aren't trying to resolve the path,
> -/// we are just trying to remove redundant things from the path.
> -///
> -/// @param[in] path
> -///     A full, partial, or relative path to a file.
> -///
> -/// @return
> -///   Returns \b true if the path needs to be normalized.
> -//------------------------------------------------------------------
> -bool needsNormalization(const llvm::StringRef &path) {
> -  if (path.empty())
> -    return false;
> -  // We strip off leading "." values so these paths need to be normalized
> -  if (path[0] == '.')
> -    return true;
> -  for (auto i = path.find_first_of("\\/"); i != llvm::StringRef::npos;
> -       i = path.find_first_of("\\/", i + 1)) {
> -    const auto next = safeCharAtIndex(path, i+1);
> -    switch (next) {
> -      case 0:
> -        // path separator char at the end of the string which should be
> -        // stripped unless it is the one and only character
> -        return i > 0;
> -      case '/':
> -      case '\\':
> -        // two path separator chars in the middle of a path needs to be
> -        // normalized
> -        if (i > 0)
> -          return true;
> -        ++i;
> -        break;
> -
> -      case '.': {
> -          const auto next_next = safeCharAtIndex(path, i+2);
> -          switch (next_next) {
> -            default: break;
> -            case 0: return true; // ends with "/."
> -            case '/':
> -            case '\\':
> -              return true; // contains "/./"
> -            case '.': {
> -              const auto next_next_next = safeCharAtIndex(path, i+3);
> -              switch (next_next_next) {
> -                default: break;
> -                case 0: return true; // ends with "/.."
> -                case '/':
> -                case '\\':
> -                  return true; // contains "/../"
> -              }
> -              break;
> -            }
> -          }
> -        }
> -        break;
> -
> -      default:
> -        break;
> -    }
> -  }
> -  return false;
> -}
> -
> -
> -}
>  //------------------------------------------------------------------
>  // Assignment operator.
>  //------------------------------------------------------------------
> @@ -252,8 +158,7 @@ void FileSpec::SetFile(llvm::StringRef p
>    }
>
>    // Normalize the path by removing ".", ".." and other redundant components.
> -  if (needsNormalization(resolved))
> -    llvm::sys::path::remove_dots(resolved, true, m_style);
> +  llvm::sys::path::remove_dots(resolved, true, m_style);
>
>    // Normalize back slashes to forward slashes
>    if (m_style == Style::windows)
> @@ -270,10 +175,10 @@ void FileSpec::SetFile(llvm::StringRef p
>    // Split path into filename and directory. We rely on the underlying char
>    // pointer to be nullptr when the components are empty.
>    llvm::StringRef filename = llvm::sys::path::filename(resolved, m_style);
> -  if(!filename.empty())
> +  if (!filename.empty())
>      m_filename.SetString(filename);
>    llvm::StringRef directory = llvm::sys::path::parent_path(resolved, 
> m_style);
> -  if(!directory.empty())
> +  if (!directory.empty())
>      m_directory.SetString(directory);
>  }
>
> @@ -424,9 +329,8 @@ bool FileSpec::Equal(const FileSpec &a,
>    // case sensitivity of equality test
>    const bool case_sensitive = a.IsCaseSensitive() || b.IsCaseSensitive();
>
> -  const bool filenames_equal = ConstString::Equals(a.m_filename,
> -                                                   b.m_filename,
> -                                                   case_sensitive);
> +  const bool filenames_equal =
> +      ConstString::Equals(a.m_filename, b.m_filename, case_sensitive);
>
>    if (!filenames_equal)
>      return false;
> @@ -712,9 +616,7 @@ bool FileSpec::IsSourceImplementationFil
>    return g_source_file_regex.Execute(extension.GetStringRef());
>  }
>
> -bool FileSpec::IsRelative() const {
> -  return !IsAbsolute();
> -}
> +bool FileSpec::IsRelative() const { return !IsAbsolute(); }
>
>  bool FileSpec::IsAbsolute() const {
>    llvm::SmallString<64> current_path;
>
>
> _______________________________________________
> lldb-commits mailing list
> lldb-commits@lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits