[Lldb-commits] [PATCH] D65237: [Support] move FileCollector from LLDB to llvm/Support

[Jan Korous via Phabricator via lldb-commits]

jkorous added inline comments.


================
Comment at: llvm/include/llvm/Support/FileCollector.h:40
+
+protected:
+  void AddFileImpl(StringRef src_path);
----------------
TLDR: private?

I'm just wondering if we could make the class safer or the correct use more obvious for classes deriving from it (if we want to support that).

The couple protected members and methods seem suggesting it's fine to use these from a derived class implementation. IIUC `m_mutex` is guarding `m_vfs_writer`, `m_seen ` and `m_symlink_map` and while it is being locked in implementation of public method `AddFile`, protected methods seem to be assuming their callers lock the mutex. Specifically there's a potential for a data race in `AddFileImpl` (calling `GetRealPath` which uses `m_symlink_map`) and in `AddFileToMapping` if either is called directly from a derived class.

Since `lldb_private::FileCollector` seems to be using only the public interface we might just declare the above private (and maybe add a doc comment when caller of a method is expected to lock the mutex)?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65237