[PATCH] D73052: [clang-tidy] RenamerClangTidy now renames dependent member expr when the member can be resolved

Fri May 8 08:33:05 PDT 2020

aaron.ballman added inline comments.

================
Comment at: clang-tools-extra/clang-tidy/utils/RenamerClangTidyCheck.h:44

+  virtual void storeCheckOptions(ClangTidyOptions::OptionMap &Opts) {}
+
----------------
njames93 wrote:
> aaron.ballman wrote:
> > I'd appreciate some comments here explaining when this should be overridden. I'd also like to understand why we need `storeOptions` and `storeCheckOptions` because the two names are so similar to one another.
> I'm still unsure what the best approach here is. `RenamerClangTidyCheck::storeOptions` needs to be called to store the `AggressiveDependentMemberLookup` Option (as well as any other renaming specific option that may be added down the line). However derived classes need to also store their respective options at the same time. Due to how c++ works there isn't a nice way to ensure base methods are called at the same time as derived methods.
> Anyway that's the reason the names are similar they do a very similar job only one is meant for the `RenamerClangTidyCheck` class and the other for derived classes.
> I agree a comment is needed to explain why so I'll add that in for now
I think it's reasonable to expect developers to call `Base::storeOptions()` from within `Derived::storeOptions()` and we don't need a second method to override. You're correct that there's not a good way to ensure the base method is called, but test cases should exist for the options in a check and so I would imagine that a developer who forgets to call the base class function will have failing test cases to help alert them to the issue. WDYT?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73052