[PATCH] D152953: [clang-tidy] Introduce fuchsia-global-variables check

[Piotr Zegar via Phabricator via llvm-commits]

PiotrZSL added inline comments.


================
Comment at: clang-tools-extra/clang-tidy/fuchsia/GlobalVariablesCheck.cpp:26
+void GlobalVariablesCheck::registerMatchers(MatchFinder *Finder) {
+  const auto is_global_or_static_candidate =
+      allOf(hasGlobalStorage(), isDefinition(), unless(isConstexpr()),
----------------
Check LLVM codding standard, use CammelCase, and put it into matcher bellow anyway.


================
Comment at: clang-tools-extra/clang-tidy/fuchsia/GlobalVariablesCheck.cpp:30
+            // Exception: references without lifetime-extension.
+            unless(allOf(hasType(references(qualType())),
+                         unless(hasInitializer(exprWithCleanups(
----------------
you can easily use here `hasType(qualType(hasCanonicalType(references`, it would work with typedefs to references.


================
Comment at: clang-tools-extra/clang-tidy/fuchsia/GlobalVariablesCheck.cpp:31-32
+            unless(allOf(hasType(references(qualType())),
+                         unless(hasInitializer(exprWithCleanups(
+                             has(materializeTemporaryExpr())))))));
+
----------------
in fact only thing that you need to check is if lifetime of materializeTemporaryExpr is extended by this varDecl, probably using getExtendingDecl () or getStroageDuration would be sufficient.



================
Comment at: clang-tools-extra/clang-tidy/fuchsia/GlobalVariablesCheck.cpp:34
+
+  Finder->addMatcher(
+      varDecl(is_global_or_static_candidate,
----------------
i would expect that this check would utilize CXXRecordDecl::hasNonTrivialDestructor().


================
Comment at: clang-tools-extra/clang-tidy/fuchsia/GlobalVariablesCheck.cpp:35
+  Finder->addMatcher(
+      varDecl(is_global_or_static_candidate,
+              unless(hasType(cxxRecordDecl(hasAnyName(IgnoreVars)))))
----------------
only classes can be non trivial to destroy, so we should exclude on this level all types that are not CXXRecordDecl.


================
Comment at: clang-tools-extra/clang-tidy/fuchsia/GlobalVariablesCheck.cpp:36
+      varDecl(is_global_or_static_candidate,
+              unless(hasType(cxxRecordDecl(hasAnyName(IgnoreVars)))))
+          .bind("global_var"),
----------------
could be better, like in other checks.


================
Comment at: clang-tools-extra/clang-tidy/fuchsia/GlobalVariablesCheck.cpp:36
+      varDecl(is_global_or_static_candidate,
+              unless(hasType(cxxRecordDecl(hasAnyName(IgnoreVars)))))
+          .bind("global_var"),
----------------
PiotrZSL wrote:
> could be better, like in other checks.



================
Comment at: clang-tools-extra/clang-tidy/fuchsia/GlobalVariablesCheck.cpp:38
+          varDecl(is_global_or_static_candidate,
+                  unless(hasType(cxxRecordDecl(hasAnyName(IgnoreVars)))),
+                  // Special handling for std::array is treated below
----------------
Caslyn wrote:
> PiotrZSL wrote:
> > what about typedefs ?
> There is the typedef scenario tested on L#90 the global-variables.cpp test and I've added an additional test for typedef to references - are there any other test cases I can add to make sure this check handles them correctly?
> 
typedef for const reference of non trivial type that is used to exend lfietime of variable (calling function that returns object with non trivial destructor).



================
Comment at: clang-tools-extra/docs/clang-tidy/checks/fuchsia/global-variables.rst:3
+
+fuchsia-global-variables
+=============================
----------------
I would name this check as: `misc-no-static-variable-non-trivial-dtor` or something similar, and then google check would just alias it as google-global-variables or something, and then fuchsia check could be named misc-`no-static-variable-non-trivial-ctor`, and current check could alias it. My main reason for that is very often awkward to use google check in project for something common, like for example explicit constructors. Those 2 rules I consider +- common, not all project need them but some project that code for DSP for example could find them useful.

You can do renaming at the end, there is script for that in repository.


================
Comment at: clang-tools-extra/docs/clang-tidy/checks/fuchsia/global-variables.rst:37-38
+
+`std::array` produces a warning only if it contains a member that is not
+trivially destructible.
+
----------------
obsolete


================
Comment at: clang-tools-extra/test/clang-tidy/checkers/fuchsia/global-variables.cpp:7
+using size_t = decltype(sizeof(int));
+
+namespace std {
----------------
Note: Remove all constructors from those classes here, we should teat this check on classes that are trivial to construct, but non trival to destroy.


================
Comment at: clang-tools-extra/test/clang-tidy/checkers/fuchsia/global-variables.cpp:35
+  // We need a destructor to make the class non trivially destructible.
+  ~NonTriviallyDestructibleClass() {}
+  int Var;
----------------



================
Comment at: clang-tools-extra/test/clang-tidy/checkers/fuchsia/global-variables.cpp:103
+ntdc_ref typedef_ref_ntdc =
+    *new NonTriviallyDestructibleClass;
+
----------------
that will act just like alias
``NonTriviallyDestructibleClass XYZ;
typedef_ref_ntdc  = XYZ;``
this ``new`` here is confusing... both examples should be made simpler.


================
Comment at: clang-tools-extra/test/clang-tidy/checkers/fuchsia/global-variables.cpp:163
+};
+
+ThreadLocal<int> global_thread_local;
----------------
what about global object in a template function:
``
template<typename T> const T& fun() {
   static T value;
   return value;
}
``
and then calling this function with both non trivial and trivial to destroy type.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152953