Page MenuHomePhabricator

Rename Minion to ASTImporterDelegate
ClosedPublic

Authored by teemperor on Apr 30 2019, 2:54 AM.

Details

Summary

I think there universal agreement that Minion isn't the best name for this class. This patch renames the class
to ASTImporterDelegate to better reflect it's goal of monitoring and extending the ASTImporter.

Diff Detail

Repository
rL LLVM

Event Timeline

teemperor created this revision.Apr 30 2019, 2:54 AM
Herald added a project: Restricted Project. · View Herald Transcript
davide accepted this revision.Apr 30 2019, 7:53 AM
davide added a subscriber: davide.

yay :)

This revision is now accepted and ready to land.Apr 30 2019, 7:53 AM
aprantl accepted this revision.Apr 30 2019, 8:42 AM
aprantl added inline comments.
lldb/include/lldb/Symbol/ClangASTImporter.h
238 ↗(On Diff #197281)

Is there some sort of doxygen comment we could add here?

martong added inline comments.Apr 30 2019, 10:57 AM
lldb/include/lldb/Symbol/ClangASTImporter.h
328 ↗(On Diff #197281)

Would it make sense to rename the variables too? E.g. m_delegates?

martong added inline comments.Apr 30 2019, 11:00 AM
lldb/include/lldb/Symbol/ClangASTImporter.h
259 ↗(On Diff #197281)

The name of the parameter is still minion ... if the goal is to completely wipe out the old name then perhaps these should be renamed. Also with the new type name it just feels odd.

shafik accepted this revision.Apr 30 2019, 2:42 PM

This is a good change!

lldb/include/lldb/Symbol/ClangASTImporter.h
259 ↗(On Diff #197281)

I fee like importer_delegate makes more sense now.

328 ↗(On Diff #197281)

or m_importer_delegates

teemperor updated this revision to Diff 197728.May 2 2019, 3:02 AM
  • Added documentation.
  • Fixed more references to 'minion' pointed out in the review.
teemperor marked 4 inline comments as done.May 2 2019, 3:03 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptMay 2 2019, 3:56 AM