labath 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;
+
----------------
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').
================
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 {
----------------
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)
================
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() ||
----------------
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("/"));
```
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
