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