[Lldb-commits] [PATCH] D130401: Implement better path matching in FileSpecList::FindCompatibleIndex(...).

[Greg Clayton via Phabricator via lldb-commits]

clayborg added inline comments.


================
Comment at: lldb/include/lldb/Utility/FileSpec.h:219-224
+  /// Get the path separator for the current path style.
+  ///
+  /// \return
+  ///     A path separator character for this path.
+  char GetPathSeparator() const;
+
----------------
labath wrote:
> clayborg wrote:
> > labath wrote:
> > > Are you sure about this part? It is my impression that we're already storing the windows paths in the "normalized" form (using `/` as separator), and so there shouldn't be a need to do anything special (at least regarding directory separators) when working with windows paths.
> > I can check to make sure by adding some unit tests and if so, I can drop this
> Cool. Can we also revert this part now that it's not necessary? (Mainly because it's misleading as there is more than one valid directory separator on windows -- that's why the old functions used the term '"preferred" separator').
yes I can.


================
Comment at: lldb/source/Core/FileSpecList.cpp:119
+      // component matches (we don't want "foo/bar.cpp" to match "oo/bar.cpp").
+      if (file_spec_case_sensitive && curr_file.IsCaseSensitive()) {
+        auto is_suffix = [](llvm::StringRef a, llvm::StringRef b) -> bool {
----------------
labath wrote:
> Change `&&` to `||`, as that's how e.g. `FileSpec::DirectoryEquals` is implemented. (I actually think `&&` makes more sense, but I think we should be consistent)
yep!


================
Comment at: lldb/source/Core/FileSpecList.cpp:121-141
+      if (file_spec_case_sensitive && curr_file.IsCaseSensitive()) {
+        if (file_spec_dir.consume_back(curr_file_dir)) {
+          if (file_spec_dir.empty() ||
+              file_spec_dir.back() == file_spec.GetPathSeparator())
+            return idx;
+        } else if (curr_file_dir.consume_back(file_spec_dir)) {
+          if (curr_file_dir.empty() ||
----------------
labath wrote:
> clayborg wrote:
> > labath wrote:
> > > This could be a lot less repetitive. Perhaps something like:
> > > ```
> > > const bool comparison_case_sensitive = file_spec_case_sensitive || curr_file.IsCaseSensitive(); // I changed this from && to || as that's the logic used elsewhere
> > > auto &is_suffix = [&](StringRef a, StringRef b) {
> > >   return a.consume_back(b) && (a.empty() || a.endswith("/"));
> > > };
> > > if (is_suffix(curr_file_dir, file_spec_dir) || is_suffix(file_spec_dir, curr_file_dir))
> > >   return idx;
> > > ```
> > Good idea
> Could you also deduplicate along the case-insenstiveness branches?
> (This is sort of where I was going with the original snippet, but then I forgot about it by the time I got to the end of it. I wanted to write something like
> ```
> bool consumed = comparison_case_sensitive ? a.consume_back(b) : a.consume_back_insensitive(b);
> return consumed && (a.empty() || a.endswith("/"));
> ```
sure thing


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D130401/new/

https://reviews.llvm.org/D130401