This is an archive of the discontinued LLVM Phabricator instance.

[libclang][index][NFCi] Refactor machinery for skipping already parsed function bodies
ClosedPublic

Authored by jkorous on Aug 23 2019, 6:01 PM.

Details

Summary

Refactor machinery for skipping inline function bodies that have already been parsed in other frontend actions.

Mostly just:

  • Changed naming with the expectation that this code is to be moved from its original context to libIndex.
  • Added couple asserts.
  • Added comments.

Diff Detail

Event Timeline

jkorous created this revision.Aug 23 2019, 6:01 PM
akyrtzi accepted this revision.Aug 24 2019, 10:44 AM
This revision is now accepted and ready to land.Aug 24 2019, 10:44 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptAug 26 2019, 10:26 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
gribozavr added inline comments.Aug 26 2019, 12:14 PM
clang/tools/libclang/Indexing.cpp
126

"SharedParsedRegions"? "ThreadSafeParsedRegions"?

134

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.

138

"addParsedRegions"?

371–372

This pointer seems to be only used in the constructor, however the constructor already has access to skData parameter.

jkorous marked 4 inline comments as done.Aug 26 2019, 12:56 PM

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?

clang/tools/libclang/Indexing.cpp
126

I'll use the ThreadSafeParsedRegions . Thanks!

134

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?

138

That's probably better. Thanks!

371–372

Are you saying that we don't need this member and can just create ParsedSrcLocationsTracker in CreateASTConsumer since we have the skData?

jkorous marked 3 inline comments as done.Aug 26 2019, 1:07 PM
jkorous added inline comments.
clang/tools/libclang/Indexing.cpp
371–372

If yes, then it's fixed.

gribozavr added inline comments.Sep 2 2019, 5:52 AM
clang/tools/libclang/Indexing.cpp
134

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.

Sorry -- you're right. I confused PPRegionSetTy with SharedParsedRegionsStorage.

Maybe you just made a case for removing the typedef?

I think that would be an improvement and happy to do that! PTAL at https://reviews.llvm.org/D67077 .