[Lldb-commits] [PATCH] D130045: Implement better path matching in FileSpecList::FindFileIndex(...).

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere added a comment.

The relative/absolute caching seems like a nice improvement. Splitting it into a separate patch would make it easier to review.

I'm slightly worried about the change to make the new "fuzzy" matching the default. While it makes sense for the breakpoints, I wouldn't generally expect `./a/b/c/main.cpp` to match `/build/a/b/c/main.cpp`, at least not unless my current working directory is `/build`. While doing the reproducers, the CWD was probably the hardest thing to get right consistently. I'm worried that this new matching is going to lead to really subtle bugs that are going to be tricky to figure out.

To make this more obvious and explicit: would it make sense to have another matches in FileSpec and have FindFileIndex take a `std::function` or `llvm::FunctionRef` with the desired matcher. Something like:

  static bool FuzzyMatch(const FileSpec &pattern, const FileSpec &file);



  size_t FileSpecList::FindFileIndex(size_t start_idx, const FileSpec &file_spec,
                                     bool full, std::function<bool(const FileSpec &, const FileSpec &)> matcher = FileSpec::Match) const {
    ...
  }



================
Comment at: lldb/include/lldb/Utility/FileSpec.h:408-409
   mutable bool m_is_resolved = false; ///< True if this path has been resolved.
+  mutable bool m_is_absolute = false; ///< True if this path is absolute.
+  mutable bool m_is_absolute_valid = false; ///< True if m_is_absolute is valid.
   Style m_style; ///< The syntax that this path uses (e.g. Windows / Posix)
----------------
Should this be an `llvm::Optional<bool>` instead or maybe even better an enum value with values `absolute, relative, unknown` or something? 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130045