[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/Core/FileSpecList.h:140-141
+  ///
+  /// \param[in] full
+  ///     Should FileSpec::Equal be called with "full" true or false.
+  ///
----------------
labath wrote:
> Is this ever being called with full=false? If not can we drop it?
Probably yes!


================
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:
> 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


================
Comment at: lldb/source/Core/FileSpecList.cpp:93
+  const bool file_spec_case_sensitive = file_spec.IsCaseSensitive();
+  // When looking for files, we will compare only the filename if the FILE_SPEC
+  // argument is empty
----------------
labath wrote:
> use lower case?
I will reword


================
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:
> 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


================
Comment at: lldb/unittests/Core/FileSpecListTest.cpp:19-21
+// static FileSpec WindowsSpec(llvm::StringRef path) {
+//   return FileSpec(path, FileSpec::Style::windows);
+// }
----------------
labath wrote:
> It would be nice to have a quick windows test as well, to confirm the separator story.
will do!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130401