[PATCH] D54222: [clang-tidy] Add a check to detect returning static locals in public headers

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

I agree with @Eugene.Zelenko that this check should rather live in `bugprone-` as the issue its diagnosing is not LLVM specific. It is ok to add an alias into the llvm module.



================
Comment at: clang-tidy/llvm/ProblematicStaticsCheck.cpp:22
+void ProblematicStaticsCheck::registerMatchers(MatchFinder *Finder) {
+  auto Matcher = returnStmt(hasReturnValue(ignoringImplicit(unaryOperator(
+                                hasOperatorName("&"),
----------------
Nit: you can inline this variable, having it does not improve readability (IMHO) nor does it reduce duplication.


================
Comment at: clang-tidy/llvm/ProblematicStaticsCheck.cpp:26
+                                    varDecl(isStaticLocal()).bind("var"))))))),
+                            isExpansionInFileMatching("/include/.*\\.h"))
+                     .bind("return");
----------------
I think the matching on `include` should not be done. Maybe it's ok to filter for header files, but we should not assume they live in `include/...` (as i think this check is generally useful and not too LLVM specific).

More filters for false positives could be checking if the function itself is `static` or in an anonymous namespace.


================
Comment at: clang-tidy/llvm/ProblematicStaticsCheck.cpp:33
+  const auto *VD = Result.Nodes.getNodeAs<VarDecl>("var");
+  const auto *Return = Result.Nodes.getNodeAs<ReturnStmt>("return");
+  diag(Return->getBeginLoc(), "address of static local variable %0 may not "
----------------
Please `assert` on `VD` and `Return`


================
Comment at: test/clang-tidy/llvm-problematic-statics.cpp:3
+
+#include "include/ProblematicStatics.h"
+// CHECK-MESSAGES: ProblematicStatics.h:7:5: warning: address of static local variable 'index' may not be identical across library boundaries [llvm-problematic-statics]
----------------
I think the testing should include more cases. At least normal functions, and a false case as well.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D54222