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