labath 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.
+ ///
----------------
Is this ever being called with full=false? If not can we drop it?
================
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;
+
----------------
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.
================
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
----------------
use lower case?
================
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() ||
----------------
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;
```
================
Comment at: lldb/unittests/Core/FileSpecListTest.cpp:19-21
+// static FileSpec WindowsSpec(llvm::StringRef path) {
+// return FileSpec(path, FileSpec::Style::windows);
+// }
----------------
It would be nice to have a quick windows test as well, to confirm the separator
story.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D130401/new/
https://reviews.llvm.org/D130401
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
