Page MenuHomePhabricator

[CrossTU][NFCI] Refactor loadExternalAST function

Authored by gamesh411 on Jul 15 2019, 9:26 AM.



Refactor loadExternalAST method of CrossTranslationUnitContext in order to
reduce maintenance burden and so that features are easier to add in the future.

Diff Detail


Event Timeline

gamesh411 created this revision.Jul 15 2019, 9:26 AM

Thank you Endre, this patch is a great initiative.
However, I think we can do better encapsulation then just the reorganization of the functions:
We could encapsulate into a nested class NameASTUnitMap and the functions which operate on this (getCachedASTUnitForName,
We could do the same with NameFileMap, lazyInitCTUIndex, getASTFileNameForLookup.
And we could also encapsulate NumASTLoaded and its related function (checkThresholdReached). In this case perhaps we could use RAII to increase the counter.

167 ↗(On Diff #209889)

Could you please provide comments for these new functions? E.g. it would be useful to know what is the IndexName param.

168 ↗(On Diff #209889)

LookupName is the USR?

185 ↗(On Diff #209889)

Could you please add comments for these maps (caches) about what we use them for?

364 ↗(On Diff #209889)

I don't think we should use auto here, because it is not obvious what will be the return type. It is important to see that we deal with an Expected<...>.
Perhaps we should have a type alias for llvm::StringMap<std::string>.

448 ↗(On Diff #209889)

This comment might be also placed in the .h file as a oneliner above the function's prototype.

gamesh411 updated this revision to Diff 211361.Jul 23 2019, 3:21 PM

Refactor functionality into local classes

Herald added a project: Restricted Project. · View Herald TranscriptJul 23 2019, 3:22 PM
gamesh411 updated this revision to Diff 211363.Jul 23 2019, 3:32 PM

Too much autoformat fixed

I have a feeling that LoadPass and LoagGuard could be (should be) merged together, other than that we are getting close.

185 ↗(On Diff #211363)

Perhaps this comment is not necessary, because it is not obvious where does the mentioned "section" ends and this is a bit redundant with the comments for the classes below.

229 ↗(On Diff #211363)

We may not need this comment.

233 ↗(On Diff #211363)

Could you please add these descriptions above the member declarations?

245 ↗(On Diff #211363)

This name CanBegin sounds strange to me. Perhaps Enabled?

251 ↗(On Diff #211363)

I have a feeling that LoadPass and LoagGuard could be (should be) merged together.

352 ↗(On Diff #211363)

I think you could leave the implementation of LoadPass:: member functions in the header, because they are so small. And that way I think code comprehension could improve.

gamesh411 marked 11 inline comments as done.Jul 24 2019, 2:47 AM

Thanks for pointing out these issues. Most of them are agreed. Merging the RAII counter with the threshold checker class, however, does not seem like a good decision for me. What would be the benefits of merging the two?

233 ↗(On Diff #211363)

Good point! Will do that.

245 ↗(On Diff #211363)

First I was thinking about making the variable names "read" as an english sentence. However CanBegin does not appear during use, so it is fair that Enable is a better name. Changing it...

251 ↗(On Diff #211363)

LoadPass is the RAII object, the other is decider whether the threshold is reached. Note also that LoadGuard owns the loaded-counter variable. IMHO these should be two separate classes, and I think if they were to be implemented in the header these two would be simple, and easy-to-understand classes.

352 ↗(On Diff #211363)

Good point!

gamesh411 updated this revision to Diff 212290.Jul 30 2019, 1:21 AM
gamesh411 marked 5 inline comments as done.
  • Merge RAII class
  • Update comments
gamesh411 marked 2 inline comments as done.Jul 30 2019, 1:27 AM

Updated the revision.
I find this solution definitely more compact with responsibilities more separated. One more thing that comes to mind is that maybe the whole explicit passing of CTUDir and IndexName arguments is a bit verbose. What are your thoughts about these being injected earlier than the query operations (eg.: in getASTUnitForFunction and getFileForFunction)?

martong added inline comments.Jul 31 2019, 5:39 AM
489 ↗(On Diff #212290)

LoadOperation should not be successful if the condition here is true.
Perhaps LoadOperation.wasSuccessful(); should be done right before the last return stmt in the function.

gamesh411 updated this revision to Diff 212788.Aug 1 2019, 5:26 AM

Specify the exact meaning of successful storage

martong accepted this revision.Aug 1 2019, 6:35 AM

LGTM! Thanks! Please do the commit.

This revision is now accepted and ready to land.Aug 1 2019, 6:35 AM
gamesh411 updated this revision to Diff 213311.Aug 5 2019, 3:55 AM
  • Remove unused member Limit
  • Rebase to current master
This revision was automatically updated to reflect the committed changes.

I discovered that there are problems with this change. The number of loaded AST units is not incremented correctly and the "CTU loaded AST file" is displayed for every access to a AST unit (even if it was got from cache). The problem is that the counting and print of AST loads does not handle the case when the load returned a cached result (and no real loading happened). This causes "annoying" messages and the AST unit limit is not handled correctly.