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