clang plugin: Put inner classes bugfix behind a flag.

clang plugin: Put inner classes bugfix behind a flag.

A follow-up to https://chromiumcodereview.appspot.com/10808078

This way, cleaning up existing style issues in inner classes doesn't
have to block a clang roll.

BUG=139346
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=148814

[Elliot Glaysher, 2012-07-27 21:09:45]

lgtm

On Fri, Jul 27, 2012 at 1:57 PM,  <thakis@chromium.org> wrote:
> Reviewers: Elliot Glaysher,
>
> Description:
> clang plugin: Put inner classes bugfix behind a flag.
>
> A follow-up to https://chromiumcodereview.appspot.com/10808078
>
> This way, cleaning up existing style issues in inner classes doesn't
> have to block a clang roll.
>
> BUG=139346
>
>
> Please review this at https://chromiumcodereview.appspot.com/10828057/
>
> SVN Base: svn://svn.chromium.org/chrome/trunk/src
>
> Affected files:
>   M tools/clang/plugins/ChromeClassTester.h
>   M tools/clang/plugins/ChromeClassTester.cpp
>   M tools/clang/plugins/FindBadConstructs.cpp
>
>
> Index: tools/clang/plugins/ChromeClassTester.cpp
> diff --git a/tools/clang/plugins/ChromeClassTester.cpp
> b/tools/clang/plugins/ChromeClassTester.cpp
> index
>
82807990635c5db7995d2955f7639b563f878028..c61271b1b93f96929c6309dee2263acee770c72b
> 100644
> --- a/tools/clang/plugins/ChromeClassTester.cpp
> +++ b/tools/clang/plugins/ChromeClassTester.cpp
> @@ -36,18 +36,24 @@ bool ends_with(const std::string& one, const
> std::string& two) {
>
>  }  // namespace
>
> -ChromeClassTester::ChromeClassTester(CompilerInstance& instance)
> +ChromeClassTester::ChromeClassTester(CompilerInstance& instance,
> +                                     bool check_inner_classes)
>      : instance_(instance),
> -      diagnostic_(instance.getDiagnostics()) {
> +      diagnostic_(instance.getDiagnostics()),
> +      check_inner_classes_(check_inner_classes) {
>    BuildBannedLists();
>  }
>
>  ChromeClassTester::~ChromeClassTester() {}
>
>  void ChromeClassTester::HandleTagDeclDefinition(TagDecl* tag) {
> -  // Defer processing of this tag until its containing top-level
> -  // declaration has been fully parsed. See crbug.com/136863.
> -  pending_class_decls_.push_back(tag);
> +  if (check_inner_classes_) {
> +    // Defer processing of this tag until its containing top-level
> +    // declaration has been fully parsed. See crbug.com/136863.
> +    pending_class_decls_.push_back(tag);
> +  } else {
> +    CheckTag(tag);
> +  }
>  }
>
>  bool ChromeClassTester::HandleTopLevelDecl(DeclGroupRef group_ref) {
> Index: tools/clang/plugins/ChromeClassTester.h
> diff --git a/tools/clang/plugins/ChromeClassTester.h
> b/tools/clang/plugins/ChromeClassTester.h
> index
>
588ae9cae5a90fedbcc2c77ec6c19b8ea5609b6e..fe11fcd21c591b43f0325e88befefea711fab84c
> 100644
> --- a/tools/clang/plugins/ChromeClassTester.h
> +++ b/tools/clang/plugins/ChromeClassTester.h
> @@ -16,7 +16,8 @@
>  // headers to subclasses which implement CheckChromeClass().
>  class ChromeClassTester : public clang::ASTConsumer {
>   public:
> -  explicit ChromeClassTester(clang::CompilerInstance& instance);
> +  explicit ChromeClassTester(clang::CompilerInstance& instance,
> +                             bool check_inner_classes);
>    virtual ~ChromeClassTester();
>
>    // clang::ASTConsumer:
> @@ -79,6 +80,9 @@ class ChromeClassTester : public clang::ASTConsumer {
>
>    // List of decls to check once the current top-level decl is parsed.
>    std::vector<clang::TagDecl*> pending_class_decls_;
> +
> +  // TODO: Remove once all inner classes are cleaned up.
> +  bool check_inner_classes_;
>  };
>
>  #endif  // TOOLS_CLANG_PLUGINS_CHROMECLASSTESTER_H_
> Index: tools/clang/plugins/FindBadConstructs.cpp
> diff --git a/tools/clang/plugins/FindBadConstructs.cpp
> b/tools/clang/plugins/FindBadConstructs.cpp
> index
>
b79a64dbd19959bc449719681212961ed39b382b..58f423ee6db2402933cd46236ecafbeecae734a5
> 100644
> --- a/tools/clang/plugins/FindBadConstructs.cpp
> +++ b/tools/clang/plugins/FindBadConstructs.cpp
> @@ -50,8 +50,9 @@ class FindBadConstructsConsumer : public ChromeClassTester
> {
>   public:
>    FindBadConstructsConsumer(CompilerInstance& instance,
>                              bool check_refcounted_dtors,
> -                            bool check_virtuals_in_implementations)
> -      : ChromeClassTester(instance),
> +                            bool check_virtuals_in_implementations,
> +                            bool check_inner_classes)
> +      : ChromeClassTester(instance, check_inner_classes),
>          check_refcounted_dtors_(check_refcounted_dtors),
>
> check_virtuals_in_implementations_(check_virtuals_in_implementations) {
>    }
> @@ -395,7 +396,8 @@ class FindBadConstructsAction : public PluginASTAction {
>   public:
>    FindBadConstructsAction()
>        : check_refcounted_dtors_(true),
> -        check_virtuals_in_implementations_(true) {
> +        check_virtuals_in_implementations_(true),
> +        check_inner_classes_(true) {
>    }
>
>   protected:
> @@ -403,7 +405,8 @@ class FindBadConstructsAction : public PluginASTAction {
>    virtual ASTConsumer* CreateASTConsumer(CompilerInstance& instance,
>                                           llvm::StringRef ref) {
>      return new FindBadConstructsConsumer(
> -        instance, check_refcounted_dtors_,
> check_virtuals_in_implementations_);
> +        instance, check_refcounted_dtors_,
> check_virtuals_in_implementations_,
> +        check_inner_classes_);
>    }
>
>    virtual bool ParseArgs(const CompilerInstance& instance,
> @@ -415,6 +418,8 @@ class FindBadConstructsAction : public PluginASTAction {
>          check_refcounted_dtors_ = false;
>        } else if (args[i] == "skip-virtuals-in-implementations") {
>          check_virtuals_in_implementations_ = false;
> +      } else if (args[i] == "skip-inner-classes") {
> +        check_inner_classes_ = false;
>        } else {
>          parsed = false;
>          llvm::errs() << "Unknown argument: " << args[i] << "\n";
> @@ -427,6 +432,7 @@ class FindBadConstructsAction : public PluginASTAction {
>   private:
>    bool check_refcounted_dtors_;
>    bool check_virtuals_in_implementations_;
> +  bool check_inner_classes_;
>  };
>
>  }  // namespace
>
>

[hans, 2012-07-27 21:24:32]

lgtm to me too, thanks

https://chromiumcodereview.appspot.com/10828057/diff/3001/tools/clang/plugins...
File tools/clang/plugins/ChromeClassTester.h (right):

https://chromiumcodereview.appspot.com/10828057/diff/3001/tools/clang/plugins...
tools/clang/plugins/ChromeClassTester.h:19: explicit
ChromeClassTester(clang::CompilerInstance& instance,
nit: no need for explicit when it takes two arguments, but i guess this is
temporary, so i guess no big deal