[PATCH] D80697: [clang-tidy] Reworked TransformerClangTidyCheck to simplify usage of Options

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

ymandel added a comment.

Thanks for the changes!

In D80697#2062517 <https://reviews.llvm.org/D80697#2062517>, @njames93 wrote:

> 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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80697