[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 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).



================
Comment at: clang-tools-extra/clang-tidy/utils/TransformerClangTidyCheck.h:44-58
-  // \p MakeRule generates the rewrite rule to be used by the check, based on
-  // the given language and clang-tidy options. It can return \c None to handle
-  // cases where the options disable the check.
-  //
-  // All cases in the rule generated by \p MakeRule must have a non-null \c
-  // Explanation, even though \c Explanation is optional for RewriteRule in
-  // general. Because the primary purpose of clang-tidy checks is to provide
----------------
This should be deprecated and then deleted after a delay (1-2 weeks) giving clients time to transition rather than breaking them right off.


================
Comment at: clang-tools-extra/clang-tidy/utils/TransformerClangTidyCheck.h:62
-  // of the language or clang-tidy options.
-  TransformerClangTidyCheck(transformer::RewriteRule R, StringRef Name,
-                            ClangTidyContext *Context);
----------------
Why delete this? It is the most commonly used constructor and cleaner than the OOP goop of the virtual method.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80697