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
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
