Page MenuHomePhabricator

[clang-tidy] Reworked TransformerClangTidyCheck to simplify usage of Options
Needs ReviewPublic

Authored by njames93 on May 28 2020, 1:13 AM.

Details

Summary

Changed TransformerClangTidyCheck to have a virtual method generate the rule. This has the advantages of making handling of any check options much simpler.

Diff Detail

Event Timeline

njames93 created this revision.May 28 2020, 1:13 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 28 2020, 1:13 AM
gribozavr2 added a subscriber: ymandel.

@ymandel owns transformers.

njames93 updated this revision to Diff 266957.May 28 2020, 11:05 AM
  • Renamed makeRule to buildRule to avoid ambiguity with tooling::makeRule

Thanks for the suggestioned change. However, I think we can do somewhat simpler. What do you think of just providing a setter on the super class

void setRule(Optional<transformer::RewriteRule> Rule) { this->Rule = std::move(Rule); }

Subclasses will call setRule in their constructor body . Or not. The code in the superclass doesn't change either way, since it already needed to check the optional. There's no initialization logic, no virtual functions, no need to delete the existing constructors (though we should deprecate them in favor of this simpler mechanism).

clang-tools-extra/clang-tidy/utils/TransformerClangTidyCheck.h
44–58

This should be deprecated and then deleted after a delay (1-2 weeks) giving clients time to transition rather than breaking them right off.

62

Why delete this? It is the most commonly used constructor and cleaner than the OOP goop of the virtual method.

njames93 updated this revision to Diff 267161.May 29 2020, 3:33 AM
njames93 marked 3 inline comments as done.
  • Add back old constructors, one being deprecated

There are a few reasons for using the virtual method:

  • It keeps everything contained in the one class.
  • In the case where options are needed it avoid extra work in the derived class.
  • It's consistent with how all base class checks handle the specifics of derived checks.
clang-tools-extra/clang-tidy/utils/TransformerClangTidyCheck.h
62

I'll add that back in, but I feel this constructor is no different in most cases to just placing the rule builder in the virtual method

Thanks for the changes!

There are a few reasons for using the virtual method:

I think I'm missing something (or I didn't explain my intention well):

  • It keeps everything contained in the one class.

Everything is still in one class -- the constructor of the derived class just calls setRule.

  • In the case where options are needed it avoid extra work in the derived class.

Why wouldn't this also avoid the extra work? The constructor of the derived class initializes the local variables and then in its body calls setRule. For example, in the class you modified above, the constructor barely changes. Just the body:

... {
  setRule(buildRule());
}

where buildRule is now just a static function are non-virtual private function and doesn't need to return optional, because the logic deciding about constructed it can just be used instead to guard the call to setRule as need be. Given the virtual method isLanguageVersionSupported, there usually won't be any such logic.

  • It's consistent with how all base class checks handle the specifics of derived checks.

Sorry, I'm not sure what you mean -- can you expand on this?