[clang-tidy] Add Zircon module to clang-tidy

[juliehockett]

Summary

Adding a Zircon module to clang-tidy for checks specific to the Zircon kernel, with a checker to fuchsia-zx (for zircon) to flag instances where specific objects are temporarily created.

[Eugene.Zelenko]

clang-tidy/fuchsia/ZxTemporaryObjectsCheck.cpp

24 ↗

(On Diff #137885)

Please include STLExtras.h and string.

clang-tidy/fuchsia/ZxTemporaryObjectsCheck.h

31 ↗

(On Diff #137885)

This contradict documentation which claims that default is empty.

docs/ReleaseNotes.rst

79 ↗

(On Diff #137885)

I think you could omit second statement.

docs/clang-tidy/checks/fuchsia-zx-temporary-objects.rst

38 ↗

(On Diff #137885)

Please use 2 spaces indentation and 80 characters limit. Continuation should be indented too.

[aaron.ballman]

clang-tidy/fuchsia/FuchsiaTidyModule.cpp

44 ↗

(On Diff #137885)

Do we want a zircon module instead? I'm wondering about people who enable modules by doing fuchsia-* and whether or not they would expect this check (and others for zircon) to be enabled.

clang-tidy/fuchsia/ZxTemporaryObjectsCheck.cpp

46 ↗

(On Diff #137885)

Elide braces.

47 ↗

(On Diff #137885)

I think this could be stated more clearly as "creating a temporary object of type %0 is prohibited" and pass in the temporary type. That will also help the user to identify what type is problematic in something like: f(good_temp{}, bad_temp{}, good_temp{});. I'm not tied to printing the type, but "misuse" suggests there's a better way to use the temporary object, which I don't think is a correct interpretation.

clang-tidy/fuchsia/ZxTemporaryObjectsCheck.h

20 ↗

(On Diff #137885)

Construction instead of constructing?

[juliehockett]

Moved check to new Zircon module, fixed comments

[aaron.ballman]

clang-tidy/tool/ClangTidyMain.cpp

527 ↗

(On Diff #138241)

You can go ahead and commit this change directly to trunk, no need to roll it in to this patch (since it's an unrelated drive-by). No review necessary.

clang-tidy/zircon/TemporaryObjectsCheck.cpp

31 ↗

(On Diff #138241)

Full stop at the end of comments (here and elsewhere).

51 ↗

(On Diff #138241)

You can skip the call to getQualifiedNameAsString(); the diagnostics engine will handle it properly.

clang-tidy/zircon/TemporaryObjectsCheck.h

21–22 ↗

(On Diff #138241)

I think the "Takes the list" sentence could be made more clear as "The list of disallowed temporary object types is configurable.", but you could probably just drop the sentence entirely as well.

docs/clang-tidy/checks/list.rst

93 ↗

(On Diff #138241)

Can you keep this list alphabetical?

docs/clang-tidy/index.rst

78 ↗

(On Diff #138241)

s/Zercon/Zircon

[juliehockett]

Addressing comments

[Eugene.Zelenko]

docs/ReleaseNotes.rst

77 ↗

(On Diff #138264)

Please sort new checks in alphabetical order.

[aaron.ballman]

I dreamed up some test cases that may or may not make sense. I think they boil down to a few things:

• Does inheriting from a prohibited type still diagnose when the derived type is used to construct a temporary?
• Should it still be prohibited if the temporary type is being returned?

clang-tidy/zircon/ZirconTidyModule.cpp

29 ↗

(On Diff #138264)

Can you add a bit of vertical whitespace before this comment?

test/clang-tidy/zircon-temporary-objects.cpp

82 ↗

(On Diff #138264)

Some additional test cases:

template <typename Ty>
Ty make_ty() { return Ty{}; }

// Call make_ty<> with two types: one allowed and one disallowed; I assume only the disallowed triggers the diagnostic.

struct Bingo : NS::Bar {}; // Not explicitly disallowed

void f() {
  Bingo{}; // Should this diagnose because it inherits from Bar?
}

// Assuming derived classes diagnose if the base is prohibited:
template <typename Ty>
struct Quux : Ty {};

void f() {
  Quux<NS::Bar>{}; // Diagnose
  Quux<Bar>{}; // Fine?
}

[juliehockett]

Addomg tests amd fixing documentation

[aaron.ballman]

test/clang-tidy/zircon-temporary-objects.cpp

86 ↗

(On Diff #138286)

Why? I thought Bar was allowed, but NS::Bar was prohibited?

95 ↗

(On Diff #138286)

The documentation should explicitly call this out, as it's different than I would have expected (I was assuming the derived classes would inherit the prohibition).

107–108 ↗

(On Diff #138286)

The comments here seem amiss.

[juliehockett]

Fixing tests and updating docs

[aaron.ballman]

clang-tidy/zircon/TemporaryObjectsCheck.cpp

51 ↗

(On Diff #138241)

This doesn't seem to be done?

docs/clang-tidy/checks/zircon-temporary-objects.rst

36–37 ↗

(On Diff #138392)

Can you hoist this into the exposition as well? It seems sufficiently important to warrant calling out in something other than an example.

[juliehockett]

clang-tidy/fuchsia/FuchsiaTidyModule.cpp

44 ↗

(On Diff #137885)

We definitely can -- it would be nice to have the additional separation (so that non-zircon parts of fuchsia don't need to explicitly disable checks.

test/clang-tidy/zircon-temporary-objects.cpp

82 ↗

(On Diff #138264)

For the inheritance ones, Quux would have to be explicitly included in the list in order to be triggered by the checker (to avoid over-diagnosing), so in this case no warnings should be generated.

86 ↗

(On Diff #138286)

Artifact of passing in the decl instead of the fully-qualified name string -- the automatic to-string method generates just the unqualified name.

[aaron.ballman]

clang-tidy/zircon/TemporaryObjectsCheck.cpp

51 ↗

(On Diff #138241)

Ah, I see why now. You should change to %q0 (and drop the existing single quotes) and then get rid of getQualifiedNameAsString().

[juliehockett]

Updating decl passed to warning string

[juliehockett]

Updating docs.

[juliehockett]

Fixing a typo sorry

[aaron.ballman]

LGTM with some minor documentation nits.

docs/clang-tidy/checks/zircon-temporary-objects.rst

7 ↗

(On Diff #138470)

Nit: the docs conflate objects and types somewhat. The configuration specifies prohibited types, and objects of that type are the problem. How about: If the object should be flagged, the fully qualified type name must be explicitly passed to the check.

36–37 ↗

(On Diff #138470)

Same problem here. How about: "This check matches temporary objects without regard for inheritance and so a prohibited base class type does not similarly prohibit derived class types."

clang-tools-extra/trunk/clang-tidy/CMakeLists.txt

 add_subdirectory(mpi)
 add_subdirectory(objc)
 add_subdirectory(performance)
 add_subdirectory(plugin)
 add_subdirectory(portability)
 add_subdirectory(readability)
 add_subdirectory(tool)
 add_subdirectory(utils)
+add_subdirectory(zircon)

clang-tools-extra/trunk/clang-tidy/tool/CMakeLists.txt

target_link_libraries(clang-tidy
   clangTidyLLVMModule
   clangTidyMiscModule
   clangTidyModernizeModule
   clangTidyMPIModule
   clangTidyObjCModule
   clangTidyPerformanceModule
   clangTidyPortabilityModule
   clangTidyReadabilityModule
+  clangTidyZirconModule
   clangTooling
   clangToolingCore
   )
 
 install(PROGRAMS clang-tidy-diff.py
   DESTINATION share/clang
   COMPONENT clang-tidy)
 install(PROGRAMS run-clang-tidy.py
   DESTINATION share/clang
   COMPONENT clang-tidy)

clang-tools-extra/trunk/clang-tidy/tool/ClangTidyMain.cpp

 static int LLVM_ATTRIBUTE_UNUSED ObjCModuleAnchorDestination =
     ObjCModuleAnchorSource;
 
 // This anchor is used to force the linker to link the HICPPModule.
 extern volatile int HICPPModuleAnchorSource;
 static int LLVM_ATTRIBUTE_UNUSED HICPPModuleAnchorDestination =
     HICPPModuleAnchorSource;
 
+// This anchor is used to force the linker to link the ZirconModule.
+extern volatile int ZirconModuleAnchorSource;
+static int LLVM_ATTRIBUTE_UNUSED ZirconModuleAnchorDestination =
+    ZirconModuleAnchorSource;
+
 } // namespace tidy
 } // namespace clang
 
 int main(int argc, const char **argv) {
   return clang::tidy::clangTidyMain(argc, argv);
 }

clang-tools-extra/trunk/clang-tidy/zircon/CMakeLists.txt

+set(LLVM_LINK_COMPONENTS support)
+
+add_clang_library(clangTidyZirconModule
+  TemporaryObjectsCheck.cpp
+  ZirconTidyModule.cpp
+
+  LINK_LIBS
+  clangAST
+  clangASTMatchers
+  clangBasic
+  clangLex
+  clangTidy
+  clangTidyUtils
+  )

clang-tools-extra/trunk/clang-tidy/zircon/TemporaryObjectsCheck.h

+//===--- TemporaryObjectsCheck.h - clang-tidy------------------*- C++ -*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_ZIRCON_TEMPORARYOBJECTSCHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_ZIRCON_TEMPORARYOBJECTSCHECK_H
+
+#include "../ClangTidy.h"
+#include "../utils/OptionsUtils.h"
+
+namespace clang {
+namespace tidy {
+namespace zircon {
+
+/// Construction of specific temporary objects in the Zircon kernel is
+/// discouraged.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/zircon-temporary-objects.html
+class TemporaryObjectsCheck : public ClangTidyCheck {
+public:
+  TemporaryObjectsCheck(StringRef Name, ClangTidyContext *Context)
+      : ClangTidyCheck(Name, Context),
+        Names(utils::options::parseStringList(Options.get("Names", ""))) {}
+  void storeOptions(ClangTidyOptions::OptionMap &Opts) override;
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+
+private:
+  std::vector<std::string> Names;
+};
+
+} // namespace zircon
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_ZIRCON_TEMPORARYOBJECTSCHECK_H

clang-tools-extra/trunk/clang-tidy/zircon/TemporaryObjectsCheck.cpp

+//===--- TemporaryObjectsCheck.cpp - clang-tidy----------------------------===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#include "TemporaryObjectsCheck.h"
+#include "../utils/OptionsUtils.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "llvm/ADT/STLExtras.h"
+#include "llvm/ADT/SmallVector.h"
+#include <string>
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace zircon {
+
+AST_MATCHER_P(CXXRecordDecl, matchesAnyName, ArrayRef<std::string>, Names) {
+  std::string QualifiedName = Node.getQualifiedNameAsString();
+  return llvm::any_of(Names,
+                      [&](StringRef Name) { return QualifiedName == Name; });
+}
+
+void TemporaryObjectsCheck::registerMatchers(MatchFinder *Finder) {
+  // Matcher for default constructors.
+  Finder->addMatcher(
+      cxxTemporaryObjectExpr(hasDeclaration(cxxConstructorDecl(hasParent(
+                                 cxxRecordDecl(matchesAnyName(Names))))))
+          .bind("temps"),
+      this);
+
+  // Matcher for user-defined constructors.
+  Finder->addMatcher(
+      cxxConstructExpr(allOf(hasParent(cxxFunctionalCastExpr()),
+                             hasDeclaration(cxxConstructorDecl(hasParent(
+                                 cxxRecordDecl(matchesAnyName(Names)))))))
+          .bind("temps"),
+      this);
+}
+
+void TemporaryObjectsCheck::check(const MatchFinder::MatchResult &Result) {
+  if (const auto *D = Result.Nodes.getNodeAs<CXXConstructExpr>("temps"))
+    diag(D->getLocation(),
+         "creating a temporary object of type %q0 is prohibited")
+        << D->getConstructor()->getParent();
+}
+
+void TemporaryObjectsCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
+  Options.store(Opts, "Names", utils::options::serializeStringList(Names));
+}
+
+} // namespace zircon
+} // namespace tidy
+} // namespace clang

clang-tools-extra/trunk/clang-tidy/zircon/ZirconTidyModule.cpp

+//===--- ZirconTidyModule.cpp - clang-tidy---------------------------------===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#include "../ClangTidy.h"
+#include "../ClangTidyModule.h"
+#include "../ClangTidyModuleRegistry.h"
+#include "TemporaryObjectsCheck.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace zircon {
+
+/// This module is for Zircon-specific checks.
+class ZirconModule : public ClangTidyModule {
+public:
+  void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
+    CheckFactories.registerCheck<TemporaryObjectsCheck>(
+        "zircon-temporary-objects");
+  }
+};
+
+// Register the ZirconTidyModule using this statically initialized variable.
+static ClangTidyModuleRegistry::Add<ZirconModule>
+    X("zircon-module", "Adds Zircon kernel checks.");
+} // namespace zircon
+
+// This anchor is used to force the linker to link in the generated object file
+// and thus register the ZirconModule.
+volatile int ZirconModuleAnchorSource = 0;
+
+} // namespace tidy
+} // namespace clang

clang-tools-extra/trunk/docs/ReleaseNotes.rst

 
 The improvements are...
 
 Improvements to clang-tidy
 --------------------------
 
 - New module ``portability``.
 
+- New module ``zircon`` for checks related to Fuchsia's Zircon kernel.
+
 - New `bugprone-throw-keyword-missing
   <http://clang.llvm.org/extra/clang-tidy/checks/bugprone-throw-keyword-missing.html>`_ check
 
   Diagnoses when a temporary object that appears to be an exception is
   constructed but not thrown.
 
 - New `cppcoreguidelines-avoid-goto
   <http://clang.llvm.org/extra/clang-tidy/checks/cppcoreguidelines-avoid-goto.html>`_ check
- The 'misc-suspicious-string-compare' check was renamed to `bugprone-suspicious-string-compare
   <http://clang.llvm.org/extra/clang-tidy/checks/bugprone-suspicious-string-compare.html>`_
 
 - The 'misc-swapped-arguments' check was renamed to `bugprone-swapped-arguments
   <http://clang.llvm.org/extra/clang-tidy/checks/bugprone-swapped-arguments.html>`_
 
 - The 'misc-undelegated-constructor' check was renamed to `bugprone-undelegated-constructor
   <http://clang.llvm.org/extra/clang-tidy/checks/bugprone-undelegated-constructor.html>`_
 
+- New `zircon-temporary-objects
+  <http://clang.llvm.org/extra/clang-tidy/checks/zircon-temporary-objects.html>`_ check
+
+  Warns on construction of specific temporary objects in the Zircon kernel.
+
 Improvements to include-fixer
 -----------------------------
 
 The improvements are...
 
 Improvements to modularize
 --------------------------
 
 The improvements are...

clang-tools-extra/trunk/docs/clang-tidy/checks/list.rst

.. toctree::
    readability-redundant-smartptr-get
    readability-redundant-string-cstr
    readability-redundant-string-init
    readability-simplify-boolean-expr
    readability-static-accessed-through-instance
    readability-static-definition-in-anonymous-namespace
    readability-string-compare
    readability-uniqueptr-delete-release
+   zircon-temporary-objects

clang-tools-extra/trunk/docs/clang-tidy/checks/zircon-temporary-objects.rst

+.. title:: clang-tidy - zircon-temporary-objects
+
+zircon-temporary-objects
+========================
+
+Warns on construction of specific temporary objects in the Zircon kernel. 
+If the object should be flagged, If the object should be flagged, the fully 
+qualified type name must be explicitly passed to the check.
+
+For example, given the list of classes "Foo" and "NS::Bar", all of the 
+following will trigger the warning: 
+
+.. code-block:: c++
+
+  Foo();
+  Foo F = Foo();
+  func(Foo());
+
+  namespace NS {
+
+  Bar();
+
+  }
+
+With the same list, the following will not trigger the warning:
+
+.. code-block:: c++
+
+  Foo F;                 // Non-temporary construction okay
+  Foo F(param);          // Non-temporary construction okay
+  Foo *F = new Foo();    // New construction okay
+
+  Bar();                 // Not NS::Bar, so okay
+  NS::Bar B;             // Non-temporary construction okay
+
+Note that objects must be explicitly specified in order to be flagged, 
+and so objects that inherit a specified object will not be flagged.
+
+This check matches temporary objects without regard for inheritance and so a
+prohibited base class type does not similarly prohibit derived class types.
+
+.. code-block:: c++
+
+  class Derived : Foo {} // Derived is not explicitly disallowed
+  Derived();             // and so temporary construction is okay
+
+Options
+-------
+
+.. option:: Names
+
+   A semi-colon-separated list of fully-qualified names of C++ classes that 
+   should not be constructed as temporaries. Default is empty.

clang-tools-extra/trunk/docs/clang-tidy/index.rst

``modernize-``         Checks that advocate usage of modern (currently "modern"
                        means "C++11") language constructs.
 ``mpi-``               Checks related to MPI (Message Passing Interface).
 ``objc-``              Checks related to Objective-C coding conventions.
 ``performance-``       Checks that target performance-related issues.
 ``portability-``       Checks that target portability-related issues that don't
                        relate to any particular coding style.
 ``readability-``       Checks that target readability-related issues that don't
                        relate to any particular coding style.
+``zircon-``            Checks related to Zircon kernel coding conventions.
 ====================== =========================================================
 
 Clang diagnostics are treated in a similar way as check diagnostics. Clang
 diagnostics are displayed by :program:`clang-tidy` and can be filtered out using
 ``-checks=`` option. However, the ``-checks=`` option does not affect
 compilation arguments, so it can not turn on Clang warnings which are not
 already turned on in build configuration. The ``-warnings-as-errors=`` option
 upgrades any warnings emitted under the ``-checks=`` flag to errors (but it

clang-tools-extra/trunk/test/clang-tidy/zircon-temporary-objects.cpp

+// RUN: %check_clang_tidy %s zircon-temporary-objects %t -- \
+// RUN:   -config="{CheckOptions: [{key: zircon-temporary-objects.Names, value: 'Foo;NS::Bar'}]}" \
+// RUN:   -header-filter=.* \
+// RUN: -- -std=c++11
+
+// Should flag instances of Foo, NS::Bar.
+
+class Foo {
+public:
+  Foo() = default;
+  Foo(int Val) : Val(Val){};
+
+private:
+  int Val;
+};
+
+namespace NS {
+
+class Bar {
+public:
+  Bar() = default;
+  Bar(int Val) : Val(Val){};
+
+private:
+  int Val;
+};
+
+} // namespace NS
+
+class Bar {
+public:
+  Bar() = default;
+  Bar(int Val) : Val(Val){};
+
+private:
+  int Val;
+};
+
+int func(Foo F) { return 1; };
+
+int main() {
+  Foo F;
+  Foo *F2 = new Foo();
+  new Foo();
+  Foo();
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: creating a temporary object of type 'Foo' is prohibited
+  Foo F3 = Foo();
+  // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: creating a temporary object of type 'Foo' is prohibited
+
+  Bar();
+  NS::Bar();
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: creating a temporary object of type 'NS::Bar' is prohibited
+
+  int A = func(Foo());
+  // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: creating a temporary object of type 'Foo' is prohibited
+
+  Foo F4(0);
+  Foo *F5 = new Foo(0);
+  new Foo(0);
+  Foo(0);
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: creating a temporary object of type 'Foo' is prohibited
+  Foo F6 = Foo(0);
+  // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: creating a temporary object of type 'Foo' is prohibited
+
+  Bar(0);
+  NS::Bar(0);
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: creating a temporary object of type 'NS::Bar' is prohibited
+
+  int B = func(Foo(0));
+  // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: creating a temporary object of type 'Foo' is prohibited
+}
+
+namespace NS {
+
+void f() {
+  Bar();
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: creating a temporary object of type 'NS::Bar' is prohibited
+  Bar(0);
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: creating a temporary object of type 'NS::Bar' is prohibited
+}
+
+} // namespace NS
+
+template <typename Ty>
+Ty make_ty() { return Ty(); }
+// CHECK-MESSAGES: :[[@LINE-1]]:23: warning: creating a temporary object of type 'Foo' is prohibited
+// CHECK-MESSAGES: :[[@LINE-2]]:23: warning: creating a temporary object of type 'NS::Bar' is prohibited
+
+void ty_func() {
+  make_ty<Bar>();
+  make_ty<NS::Bar>();
+  make_ty<Foo>();
+}
+
+// Inheriting the disallowed class does not trigger the check.
+
+class Bingo : NS::Bar {}; // Not explicitly disallowed
+
+void f2() {
+  Bingo();
+}
+
+template <typename Ty>
+class Quux : Ty {};
+
+void f3() {
+  Quux<NS::Bar>();
+  Quux<Bar>();
+}