[PATCH] D66694: [libclang][index][NFCi] Refactor machinery for skipping already parsed function bodies

[Jan Korous via Phabricator via cfe-commits]

jkorous marked 4 inline comments as done.
jkorous added a comment.

Hmm, I already landed this - let's transfer the discussion to https://reviews.llvm.org/D66764.
I'll make the changes to that patch. Is that ok?



================
Comment at: clang/tools/libclang/Indexing.cpp:126
+/// Is thread-safe.
+class SharedParsedRegionsStorage {
   std::mutex Mux;
----------------
gribozavr wrote:
> "SharedParsedRegions"? "ThreadSafeParsedRegions"?
I'll use the `ThreadSafeParsedRegions `. Thanks!


================
Comment at: clang/tools/libclang/Indexing.cpp:134
   void copyTo(PPRegionSetTy &Set) {
     std::lock_guard<std::mutex> MG(Mux);
     Set = ParsedRegions;
----------------
gribozavr wrote:
> I think we should lock both the source and destination mutexes (use `std::scoped_lock` that allows to lock multiple mutexes).
> 
> Also, it would be more idiomatic to express this API as a copy constructor and an assignment operator.
Sorry, I am not following. You are aware of the argument being a `DenseSet`, right? This doesn't look like a named copy-ctor to me.

The way this works is that `ParsedSrcLocationsTracker` takes snapshot of the `SharedParsedRegionsStorage` in its constructor (using `copyTo()`), then uses its own local data and synchronizes the local data with `SharedParsedRegionsStorage` in `syncWithStorage()` method.

Maybe you just made a case for removing the typedef?


================
Comment at: clang/tools/libclang/Indexing.cpp:138
 
-  void update(ArrayRef<PPRegion> Regions) {
+  void merge(ArrayRef<PPRegion> Regions) {
     std::lock_guard<std::mutex> MG(Mux);
----------------
gribozavr wrote:
> "addParsedRegions"?
That's probably better. Thanks!


================
Comment at: clang/tools/libclang/Indexing.cpp:371
 
-  SessionSkipBodyData *SKData;
-  std::unique_ptr<TUSkipBodyControl> SKCtrl;
+  SharedParsedRegionsStorage *SKData;
+  std::unique_ptr<ParsedSrcLocationsTracker> ParsedLocsTracker;
----------------
gribozavr wrote:
> This pointer seems to be only used in the constructor, however the constructor already has access to `skData` parameter.
Are you saying that we don't need this member and can just create `ParsedSrcLocationsTracker` in `CreateASTConsumer` since we have the `skData`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66694